Using range minimal query for lowest common ancestor (LCA) Here is my code to use range minimal query to resolve the LCA problem. Any advice on code bugs, performance improvement in terms of algorithm time complexity, code style are appreciated.

More specifically, my questions are:

I am using in-order traverse to build depth array. Should I use other traverse, like pre-order (dfs), level-order, or post order?
I am using two additional data structures, index_node_map and node_index_map. I'm wondering if there are any ways to remove them, as they make my code seem lengthy.

Major ideas of my idea are:

Build depth array of each node by in-order traverse
Build MRQ index for depth array built in first step
For any two nodes, their LCA are the lowest depth node between them (in depth array)

from collections import defaultdict
class TreeNode:
    def __init__(self, value, left, right):
        self.value = value
        self.left = left
        self.right = right
class Solution:
    def __init__(self):
        self.index_node_map = defaultdict(int)
        self.node_index_map = defaultdict(int)
        self.depth = []
        self.rmp_map = defaultdict(tuple) # key: (index, step), value: (min value, index)
    def in_order(self, root, level):
        if root.left:
            self.in_order(root.left, level+1)
        self.depth.append(level)
        self.index_node_map[len(self.depth)-1]=root.value
        self.node_index_map[root.value] = len(self.depth)-1
        if root.right:
            self.in_order(root.right, level+1)
    def build_depth_index(self, root, level):
        self.in_order(root, level)
        print self.depth
        print self.index_node_map
        print self.node_index_map
        self.build_rmq_map(self.depth)

    def build_rmq_map(self, result):
        step = len(result).bit_length()
        for i in range(len(result)):
            self.rmp_map[(i,0)]=(result[i],i)
        for s in range(1, step+1):
            for i in range(len(result)):
                if i + 2 **(s-1) < len(result):
                    self.rmp_map[(i,s)] = min(self.rmp_map[(i,s-1)],self.rmp_map[(i+2**(s-1),s-1)])
                else:
                    self.rmp_map[(i,s)] = self.rmp_map[(i, s-1)]

    def rmp_query(self, index1, index2):
        step = (index2 - index1).bit_length()-1
        x = self.rmp_map[(index1, step)]
        y = self.rmp_map[(index2+1-2**(step), step)]
        # return index of depth array min value
        if x[0] < y[0]:
            return x[1]
        else:
            return y[1]

    def lca_query(self, node1, node2):
        index1 = self.node_index_map[node1]
        index2 = self.node_index_map[node2]
        min_index = self.rmp_query(index1, index2)
        return self.index_node_map[min_index]

if __name__ == "__main__":
    root = TreeNode(1, TreeNode(2, TreeNode(4, None, None), TreeNode(5, None, None)), TreeNode(3, TreeNode(6, None, None),TreeNode(7,None,None)))
    s = Solution()
    s.build_depth_index(root, 0)
    print s.lca_query(4,5)
    print s.lca_query(4, 7)

This review describes my thoughts as a fellow competitive programmer.

About your questions

In-order traversal is a must here. Otherwise the RMQ-based algorithm won't work - it requires that for each vertexes A and B their LCA occurs between any occurrences of A and B.
You will need some kind of maps to map tree node to their position in the RMQ array. So, at least one of them should stay. However, you can get rid of index_node_map if you make RMQ store node's "value" (which is actually node's id, as far as I understood from your code).

Major problems

Your code does not satisfy PEP 8 at least in terms of whitespaces (it's easy to check and fix with tools like pep8, flake8, autopep8), that significantly reduces readability.
You have God object in the solution, which, well, "solves the problem". Meaning, it mixes together all of the following:

In-order traversal of a tree.
Mapping between tree nodes and their indexes in the traversal.
Calculating depths of vertexes.
Reducing LCA to RMQ with
Solution of RMQ problem with sparse table.

Moreover, all these are highly tied together and you cannot, say, debug RMQ separately. It is a very common problem among competitive programmers (especially beginners). Though, some do not consider it to be a problem, but you're posting on codereview.
You initialize object outside of constructor, in the build_depth_index function. It may lead to a false conclusion that one may re-use the object with different inputs by calling build_depth_index with new tree. Despite "work in the constructor" being a kind of anti-pattern, I feel it's OK to move all initialization to the constructor in this case:

You won't be able to reuse or not initialize the object.
You will have less mutable state to care about (as opposed to the "initialize everything in build_depth_index and make the object reusable" approach), which is generally helpful for debugging.

There are a bunch of implicit invariants, e.g.:

You assume that all values in the tree are different. It's not enforced with asserts, so it's possible to accidentally apply your algorithm to "wrong" tree and get "almost correct" results.
In the in_order function you assume that each vertex occurs in the in-order traversal only once (because you "reenumerate" vertexes), but it's not the case for non-binary trees. Algorithm is more general than that. I'd suggest to avoid writing general algorithms for some "specific" cases because that is prone to copy-pasting and makes the algorithm harder to debug.

There is some debug output in the middle, which cannot be switched off.
No need in defaultdicts. I'd suggest to avoid defaultdicts when implementing data structures (esp. RMQ) because typically you should initialize the structure first and use it afterwards.

  self.rmp_map = defaultdict(tuple) # key: (index, step), value: (min value, index)

you represent two-dimensional array with dict. Please don't do that, use arrays.
Use namedtuple instead of regular tuples to improve readability.

My suggestions

Get rid of God object

I'd suggest the following objects and functions:

class Rmq:
    def __init__(self, array, key):
        """Initializes RMQ structure for 'array', assuming that the value
        being minimized is returned by the 'key' function, if applied to
        array element"""

    def get_minimal_element(self, l, r):
        """Returns minimal element of array from position l to r (inclusive)"""

def get_in_order_traverse(tree):
    """Traverses tree in-order and returns a list of nodes."""

def get_depths(tree):
    """Calculates depths of all vertexes in the tree and returns mapping
    from vertex's value to its depth (root's depth is 0)."""

class Lca:
    def __init__(self, tree):
        """Initializes LCA solver for the given tree."""

    def get_lca(self, a, b):
        """Given two vertexes a and b returns their LCA as a TreeNode."""

Of course, that design can be de-coupled even further. Say, Lca.__init__ can take an optional argument specifying Rmq's factory so we can plug different implementations of Rmq for debugging.

Other suggestions

Rename TreeNode.value to TreeNode.id, because LCA does care about node "values", it cares about their ids only.
Even better approach is to make TreeNodes hashable by overloading __hash__ and __eq__ so functions do not depend anymore on "what is node's id and how to find it".
Replace TreeNode.left and TreeNode.right with a list called TreeNode.children. For the sake of generality - algorithm does not care.
Use logging module for debug output so it can be switched on/off dynamically.
Use namedtuple instead of tuples.
In your sparse table implementation you can ignore all those segments which extend beyond array's length and get rid of if. You never need to access them, as all requests to RMQ lie inside the original array.


List of TV show episodes, with next/previous episode links for each entry I'm a beginner yet, and currently I'm making a site about The Flash (just to practice), and yesterday I figured out this code to make my next/previous (episode) buttons work correctly. I'm glad I found a way after hours, but unfortunatelly it's pretty long.

So I'm wondering if someone could offer me a shorter code for this? How could I simplify it?

HTML code

I have 23 of these divs

<button id="episode1none">-</button>
<button id="episode1next" onclick="S1E2()">Next episode</button>

<button id="episode2previous" onclick="S1E1()">Previous episode</button>
<button id="episode2next" onclick="S1E3()">Next episode</button>

<button id="episode3previous" onclick="S1E2()">Previous episode</button>
<button id="episode3next" onclick="S1E4()">Next episode</button>

Css is not important, all I did there, that I set a default display:none; for these divs

and the JS

function S1E1(){
    document.getElementById("episode1buttons").style.display="block";
    document.getElementById("episode2buttons").style.display="none";
    document.getElementById("episode3buttons").style.display="none";
    document.getElementById("episode4buttons").style.display="none";
    document.getElementById("episode5buttons").style.display="none";

function S1E2(){
    document.getElementById("episode2buttons").style.display="block";
    document.getElementById("episode1buttons").style.display="none";
    document.getElementById("episode3buttons").style.display="none";
    document.getElementById("episode4buttons").style.display="none";
    document.getElementById("episode5buttons").style.display="none";

and there are more, but unecessary codes to them yet

I'm not sharing much code, because I don't want to make it too long

There are 23 functions like this, and in each of them, I set display:none; for 22 div-s, and display:block; for the one that contains the proper buttons that belong to the episode

And it's working, but it's pretty long(overall:529 lines of codes)

Feel free to ask if I missed something or you don't understand something!

Some points

Event listeners

When adding event listeners it is considered bad practice to add them in the page content.

You had

<button id="episode1next" onclick="S1E2()">Next episode</button>

Instead use addEventListener

const elem = document.getElementById("episode1next");
elem.addEventListener("click",S1E2); // remove the "on" from event name
                                     // point to the function (no () after function name)
                                     // DONT call the function (has () after function name)

Naming

Use a well defined naming convention, and use it consistently.

Each language has a set of conventions that define how you create names for various things. In JavaScript we use lower camelCase for everything except objects that you create with the new token, for which we use upper CamelCase

Unfortunately JavaScript shares its environment with the DOM (HTML and CSS) and they have a completely different naming convention called BEM, which is incompatible with JS (you can not use BEM names in JS as they break syntax rules) Article regarding BEM and CSS

Ideally you use the correct naming convention for the correct content and learn how to convert between them (when its automatic, when not automatic, and the exceptions)  eg CSS font-size becomes JS fontSize, element data attributes are automatically camelCased, ,any element attribute values (like ids) are not converted and may need to use indirect queries or bracket referencing.)

You will note that these are conventions. Some (like me) opt to follow only one convention. JS camelCase, using it in the DOM and CSS for all names I define. This is not main stream.

You have followed no apparent naming convention and that will make it very hard to know when and where to put capitals, '-' etc. You remember a variable by name, that for mere mortals does not include the naming format.

Use class

If you find your self setting class properties directly to elements you should give a moments time and consider if it is done better using a CSS rule.

You do the following

  document.getElementById("episode1buttons").style.display="block";
     document.getElementById("episode2buttons").style.display="none";

But it would be better if you create a CSS rule hide and apply that rule to the elements class

 /* CSS rule in CCS */
 .hide { display : none; }

 // Unhide and hide elements in JS
 document.getElementById("episode1buttons").classList.remove("hide");
 document.getElementById("episode2buttons").classList.add("hide");

A generic event listener

You have a list of objects related to a TV show. You have coded it by hand for each particular episode which is laborious, prone to error, and just a painful time consuming process.

Using your naming we can improve the code complexity by having only one event listener that can deduce what the click is for and what to do with it.

For this we wrap everything that needs a click event in a containing element ( element id episodes) For each clickable element we add data we can use to determine what to do with the click. (data-clickType and data-episode)

We set up the initial visibility of episodes in the HTML.

    <button id="episode1none" data-clickType="none">-</button>
    <button id="episode1next" data-clickType="next" data-episode="1">Next episode</button>

    <button id="episode2previous" data-clickType="prev" data-episode="2">Previous episode</button>
    <button id="episode2next" data-clickType="next" data-episode="2">Next episode</button>

    <button id="episode3previous" data-clickType="prev" data-episode="3">Previous episode</button>
    <button id="episode3next" data-clickType="next"  data-episode="3">Next episode</button>

**strong text**

Then in javascript add an event listener to the containing element

episodes.addEventListener("click",episodeClick);

And create a listener function that works out what to do with the click

function episodeClick(event){
   var dir = 0; // forward or backward
   if (event.target.dataset.clickType === "prev") { dir = -1 }
   if (event.target.dataset.clickType === "next") { dir =  1 }
   // only if there is a dir
   if (dir) {
       const epNumber = Number(event.target.dataset.episode); // convert to a number
       let elementName = `episode${epNumber}buttons`; // get element id to hide
       // hide the element
       document.getElementById(elementName).classList.add("hide"); // hide this element
       epNumber += dir; // get next or prev episode
       elementName = `episode${epNumber}buttons`; // get element id to show
       // show the element
       document.getElementById(elementName).classList.remove("hide");
   }
}

Now you can add to the HTML as many episodes as you like, and don't need to change the code saving you a lot of time, and reducing the chance of bugs creeping in due to typos just because there has been a content change.


Is there a more efficient way of executing multiple SQL prepared statements at once? I'm updating more than one table at once and, as you will see, it uses a lot of similar looking code. Is there a more efficient way of doing this, perhaps some way of combining the queries?

For context: A user submits a small form containing information about their age, gender and favorite genre of games.

String sex, age;
String[] checkboxes = request.getParameterValues("genre");

sex = request.getParameter("sex");
age = request.getParameter("age");

try(Connection con = ds.getConnection()){
    PreparedStatement updateSex = null;
    PreparedStatement updateAge = null;
    PreparedStatement updateGenre = null;

    String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";
    String updateAgeString = "UPDATE age SET count=count+1 WHERE id=?";
    String updateGenreString = "";

    for(int i = 0; i < checkboxes.length; i++){

        updateGenreString = "UPDATE genre SET count=count+1 WHERE id=?";
        updateGenre = con.prepareStatement(updateGenreString);
        updateGenre.setString(i+1, checkboxes[i].toString());
        updateGenre.executeUpdate();
        updateGenre.close();
    }

    updateSex = con.prepareStatement(updateSexString);
    updateSex.setString(1, sex);
    updateSex.executeUpdate();
    updateSex.close();

    updateAge = con.prepareStatement(updateAgeString);
    updateAge.setString(1, age);
    updateAge.executeUpdate();
    updateAge.close();

} catch(SQLException e){
    e.printStackTrace();
}

Declaring Variables and other things:

try (Connection con = ds.getConnection()) {
    PreparedStatement updateSex = null;
    PreparedStatement updateAge = null;
    PreparedStatement updateGenre = null;

    String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";
    String updateAgeString = "UPDATE age SET count=count+1 WHERE id=?";
    String updateGenreString = "";

In your current approach, multiple prepared statements are not helping anyone..
You can just put the new prepared statements you are performing into the same PreparedStatement currentStatement.

This cuts down your declarations by quite a bit. Next are the update*String variables you have here. These things are Constants. Don't declare them on method-level, they can be reasonably expected to not change. Instead these should be classwide constants:

public class ... {
    private static final String UPDATE_SEX_STRING = "UPDATE sex SET count=count+1 WHERE name=?";
    private static final String UPDATE_AGE_STRING = "UPDATE age SET count=count+1 WHERE id=?";
    // you get the gist ;)

The small and the big wtf:

String updateGenreString = "";

for(int i = 0; i < checkboxes.length; i++){

    updateGenreString = "UPDATE genre SET count=count+1 WHERE id=?";
    updateGenre = con.prepareStatement(updateGenreString);
    updateGenre.setString(i+1, checkboxes[i].toString());

Why didn't you move the assignment to the updateGenreString, that doesn't even change to the declaration?

Additionally: Why is the parameter index depending on your loop counter? That makes absolutely no sense, because in case you have more than a single checkbox, things will break with the current statement!
If there's no second parameter, what happens when you try to set it? You get a SQLException, at least per the JavaDoc.

And another one: Why is the id a String? In general using Strings as ID/UID/UUID is strongly advised against. You're losing out on fast searchability and a few other things.

The id in the age-table also seems to be a String, at least according to the method you call.

Bottom line: Your database schema seems skewed.


User Authentication/Permissions for PHP MySQL CRUD I took my first stab at a user authentication function. I invented my own way of doing this. Is this a safe and efficient way of giving a user Read/Edit/Delete permission on tables in a MySQL database?

The Class

class Personnel {
//set tables that will need permissions and actions
protected static $actions = array("read", "edit", "delete");
protected static $table_names = array("species", "users", "news");

//this will generate a form with checkbox for each position in the permissions array
public static function set_permissions_form($current_permissions) {

    for($i=0; $i<count(self::$table_names); $i++) {
        $table_name = self::$table_names[$i];
        $form .= $table_name . ":
";
        for($z=0; $z<count(self::$actions); $z++) {
            $action = self::$actions[$z];
            $form .= $action . ": <input type='checkbox' name='permissions[" . $table_name . "][" . $action . "]'";
            if($current_permissions) {
                $form .= ($current_permissions[$table_name][$action] == "1" ? " checked =\"checked\"" : "");
            }
            $form .= " value=1 />";
            $form .= "
";
        }
    }
    return $form;
}
}

The Form

If this is a new user or an existing user with no permissions set then $current_permissions will be NULL. If a permissions array does exist in the database unserialize it and check any checkbox when name = array key.

$current_permissions = unserialize($personnel->permissions);
$form = "<form action='save_personnel.php' method='post'>";
$form .= Personnel::set_permissions_form($current_permissions);
$form .= "<input type='submit' value='Save Changes' />";
$form .= "</form>";
echo $form;

Save

Once the form is submitted each box checked will be a "1" in the permissions array. This next snippet serializes the array and stores it in the database. I did not include code for $personnel->save() but you know what it does. Create user if does not exist, update user if it does.

$personnel = new Personnel();
$personnel->permissions = serialize($_POST['permissions']);
if($personnel->save()) {
    redirect_to("manage_personnel.php");
}

Check for Permission

The next steps I haven't completed yet but basically it will pull $personnel->permissions from database and unserialize it. Then say it is on a page where user is trying to edit species, $permissions["species"]["read"] == '1' ? you shall pass : you shall not pass;.

Am I heading in the right direction with this or am I over complicating?

Loops

Why are you using a for loop? I would just use a foreach loop, its much more efficient, especially for what you are doing here.

foreach(self::$table_names as $table_name) {
    //etc...
}

Strings

The following are micro-optimizations, but ones that are usually accepted as standard practice.

Single quotes should be used if no PHP processing is to be performed on a string. In other words, there are no PHP variables in the string. This is a micro optimization because it doesn't really add any noticeable overhead to your script. However, doing this you'll notice that you wont have to escape quotes nearly as frequently as you would have to if you started with single quotes. This is especially handy when dealing with HTML or XML attributes, which I'll get into soon.

$form .= $table_name . ':
';

Double quotes should be used for strings that require PHP processing.

$form .= "$table_name:
";

HTML/XML Attributes

While most browsers accept it, it is still improper syntax to use single quotes on HTML/XML tag attributes. XML will actually produce errors. Either escape the double quotes or escape the string and only go back into PHP for the variables.

$form .= $action . ": <input type=\"checkbox\" name=\"permissions[$table_name][$action ]\"";
//OR
<input type="checkbox" name="permissions[<?php echo $table_name; ?>][<?php echo $action; ?>]"

Sanitizing and Validating

You shouldn't handle raw POST data. Always sanitize and validate it. Check out PHP's filter_input() function. That goes right along with what Shaad said about passing serialized data to your tables too. Its not a good idea, especially if it hasn't been sanitized and validated.

Comparisons

I would use absolute equality comparisons when checking for current permissions rather than loose comparison as your $actions table should only have boolean values. Also the quotes around an integer is unnecessary, unless you are comparing a string with absolute equality.

$form .= ($current_permissions[$table_name][$action] === TRUE ? " checked =\"checked\"" : "");


C pointer based growable stack rev2 Following on from my previous question: C pointer based growable stack

I have made some improvements (hopefully!) based on the very helpful comments and suggestions.
I have checked that the stack can push and pop doubles now, so hopefully it will also be OK with structs.

I have a could of questions regarding the changes I have made:

I noticed memcpy() didn't work with void * so I switched to using uin8_t; is that recommended/necessary or could I have cast to uint8_t for memcpy()? Not sure which approach is the best...
How should a failed malloc()/realloc() be handled? Return an error code? Exit? Return NULL?

Again, I would greatly appreciate any suggestions/criticism:

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

#define MAX_ITEMS 16

typedef struct {
    uint8_t *data;
    size_t itemSize;
    unsigned count;
    unsigned capacity;
} Stack;

void stack_init(Stack *stack, size_t itemSize, unsigned capacity);
bool stack_is_empty(const Stack *stack);
void stack_push(Stack *stack, void *item);
void* stack_pop(Stack *stack);
void stack_destroy(Stack *stack);

void stack_init(Stack *stack, size_t itemSize, unsigned capacity)
{
    unsigned initialCapacity = capacity == 0 ? 1 : capacity;
    size_t size = initialCapacity * itemSize;

    stack->count = 0;
    stack->capacity = initialCapacity;
    stack->itemSize = itemSize;
    stack->data = (uint8_t *)malloc(size);

    if (stack->data == NULL)
    {
        // TODO
    }

    memset(stack->data, 0, size);
}

bool stack_is_empty(const Stack *stack)
{
    return stack->count == 0;
}

void stack_push(Stack *stack, void *item)
{
    if (stack->count >= stack->capacity)
    {
        stack->capacity *= 2;
        stack->data = (uint8_t *)realloc(stack->data, stack->capacity * stack->itemSize);

        if (stack->data == NULL)
        {
            // TODO
        }
    }

    unsigned offset = stack->count * stack->itemSize;
    memcpy(stack->data + offset, item, stack->itemSize);
    stack->count++;
}

void* stack_pop(Stack *stack)
{
    if (stack_is_empty(stack) == true)
    {
        // TODO
    }

    uint8_t *item = (uint8_t *)malloc(stack->itemSize);

    if (item == NULL)
    {
        // TODO
    }

    stack->count--;
    unsigned offset = stack->count * stack->itemSize;
    memcpy(item, stack->data + offset, stack->itemSize);

    return (void *)item;
}

void stack_destroy(Stack *stack)
{
    free(stack->data);
    stack->count = 0;
}

int main(void)
{
    Stack stack;
    stack_init(&stack, sizeof(int), 0);

    for (int i = 0; i < MAX_ITEMS; i++)
    {
        stack_push(&stack, (void *)&i);
    }

    while (!stack_is_empty(&stack))
    {
        int value;
        value = *((int *)stack_pop(&stack));
        printf("%d\n", value);
    }

    stack_destroy(&stack);
}

stack->data = (uint8_t *)realloc(stack->data, stack->capacity * stack->itemSize);

This is wrong, because if realloc() failed, then there's now no way to access the memory previously pointed to by stack->data.  The correct pattern for realloc() is:

void *newData = realloc(stack->data, stack->capacity * stack->itemSize);
if (!newData) {
    // Insert error handling - you'll want to restore the old stack->capacity here
    // or (better) wait until we're okay before updating.
    return FAILURE:
}
stack->data = newData;

I recommend against casting the result of malloc() family when assigning - these functions return void*, which is assignable to any pointer type without a cast.  The unnecessary cast just distracts the reader (because casts generally indicate danger areas in code).  The same is true for conversions from pointer types to void*, such as this one:

return (void *)item;

That can simply be

return item;

The stack_pop interface is difficult to use - callers have to deal with possible null pointer return, and must remember to free() the result.  Instead, consider having the caller provide memory to write into:

void stack_pop(Stack *stack, void *target);

The caller should already know the size required for target, and can now get  results in local (auto) storage:

int foo;
stack_pop(&stack, &foo);
// no need to check for null, or to call free()


Is it a good practice to wrap datatypes in custom classes? So in my current application, I was thinking to make a extended class for all the datatypes. For int, it will be something like ExtendedInt, for bool it will be something like ExtendedBool.

These individual extended classes will expose some special functonality bound to that type. For example, this is how i think my ExtendedInt will look like:

public interface IExtended
{
   bool IsModified { get; set; }
}

Public class ExtendedInt: IExtended
{
   private bool m_IsModified;
   private int m_MaxValue = int.MaxValue ;
   private int m_MinValue = int.MinValue;
   private int m_Value=int.MinValue;
   private int m_MaxValue = int.MaxValue ;
   private int m_MinValue = int.MinValue;

   public bool IsModified
      {
    get { return m_IsModified; }
    set { m_IsModified = value; }
      }

    public int Value
    {
        get { return m_Value; }
        set
    {
    // Only change the value if it is different.

             if (m_Value != value)
       {
          // Only set the value if it is in the valid range, if any max/min exists.
          if ((m_MaxValue > -1) && (value > m_MaxValue))
          {
              throw new ArgumentException("Value is above maximum value permitted.");
           }
           else if ((m_MinValue > -1) && (value < m_MinValue))
           {
               throw new ArgumentException("Value is below minimum value permitted.");
           }
           else
           {
              m_Value = value;
              this.IsChanged = true;
              if (m_Parent != null) m_Parent.DirtyState();
            }
               }
           }
       }

Similarly, I will provide ExtendedBool, ExtendedDouble, ExtendedDate, ExtendedString, etc..

Is this considered the best practice or it is overhead for my application?

Too much overhead - nullable types and extension methods seem to give you what you need. The checking you do on your setters will never be executed because an int will never be less than int.MinValue or greater than int.MaxValue to begin with.

Only create wrappers over types where you're adding intrinsic business (or technical) value that your consumers will benefit from. I'm afraid the ExtendedInt example you provide doesn't seem to meet that criteria over int? myInt;


Flood game implementation There is the implementation of flood game on my GitHub.

The app.coffee is the main part of project:

class Square
  colors = [
    'orange',
    'green',
    'red',
    'blue',
    'violet',
    'yellow',
  ]
  constructor: (@game, @node)->
    @node.on 'click', =>
      @game.flood(@color)

  reset: ->
    @controlled = false
    color = colors[Math.floor(Math.random() * colors.length)]
    @setColor(color)

  setColor: (color)->
    @node.removeClass(@color)
    @color = color
    @node.addClass(@color)

class Game
  DIMENSION = 14
  SIZE = DIMENSION * DIMENSION
  constructor: ($tbody)->
    @limit = 25
    @grid = []
    for i in [0...DIMENSION]
      $tr = $ '<tr/>'
      $tbody.append $tr
      @grid.push []
      for j in [0...DIMENSION]
        $td = $('<td/>').html('&nbsp;')
        $tr.append $td
        @grid[i].push(new Square(@, $td))
    @top = @grid[0][0]
    @reset()

  reset: ->
    for i in [0...DIMENSION]
      for j in [0...DIMENSION]
        @grid[i][j].reset()
    @top = @grid[0][0]
    @top.controlled = true
    @flood(@top.color)
    @setTurnCounter(0)

  setTurnCounter: (counter)->
    $counter = $ '#count'
    @turn = counter
    $counter.toggleClass 'bad', @turn > @limit
    $('#counter-used').text(@turn)

  flood: (color)->
    if @top.color is color
      return
    @setTurnCounter(@turn + 1)
    @_flood(0, 0, color, [])
    if @hasWon()
      setTimeout(
        =>
          @decreaseLimit()
          @reset()
        2000
      )
  decreaseLimit: ()->
    @expected--

  _flood: (i, j, color, checked)->
    if i < 0 or j < 0 or i == DIMENSION or j == DIMENSION
      return
    cur = @grid[i][j]
    if cur.controlled or cur.color is color
      if cur in checked
        return
      cur.setColor(color)
      cur.controlled = true
      checked.push cur
      @_flood(i-1, j, color, checked)
      @_flood(i+1, j, color, checked)
      @_flood(i, j-1, color, checked)
      @_flood(i, j+1, color, checked)

  hasWon: ->
    for i in [0...DIMENSION]
      for j in [0...DIMENSION]
        if not @grid[i][j].controlled
          return false
    return true

(($, Game)->
  tbody = $('tbody')
  game = new Game(tbody)
  btnRestart = $('#btn-restart')
  btnRestart.on 'click', (e)->
    e.preventDefault()
    game.reset()

)(jQuery, Game)

I build assets with Grunt and use grunt-serve as a simple HTTP server.

P.S.: I use styles from another implementation of this game but based on Knockout.js.

Overall, it's not bad. I only have some minor observations on the code itself:

You're setting, and re-setting @top in both the constructor and reset.
SIZE isn't used for anything.
Why does reset call flood? As far as I can tell, there's no need at all.
Doesn't seem like decreaseLimit has a point.
You could postfix a couple of if/unless statements and cut down on indentation.
in _flood the if i < 0 or j < 0 or i == DIMENSION or j == DIMENSION check could be written as:

return unless 0 <= i < DIMENSION and 0 <= j < DIMENSION

or you could simply skip it in favor of trying the coordinates and checking afterward:

square = @grid[i]?[j]
return unless square?

When looping through the @grid (in reset and hasWon) there's no need to use a [0...DIMENSION] range. You can just use regular CoffeeScript for...in loops:

for row in @grid
  for square in row
    # do something with square...

The IIFE at the end could be written more idiomatically using the do keyword

do ($ = jQuery, Game) ->
  ...

Structurally, I have a few notes:

You inject both the Game object and a td element into Square, so Square doesn't really have that much to do. In fact, the first thing it does is set an event listener on the element that the game object created, and set it to call back to the game object. It just seems a roundabout way to set things up.
I'd probably move the td creation to Square.
You pass a tbody element to the Game constructor, which is nice. But Game also directly accesses #count and #counter-user. So it's not really decoupled from the markup at all.
Somewhat similarly: You pass in jQuery as $ in the IIFE that kicks things off. That's a perfectly good pattern. However, Game just uses $ "raw". So you're sort of half-way following the pattern.

Here's a (simplified) refactored version, just for fun.


Filtering data stored using classes via user-called function the proper way? I'm currently working on an API package. Basically what it does is scrapes team data and stores them in list of Team objects. The team object contains data such as Team.name,rank, country etc.

If a user wants to filter that list to find something in it, I have this really messy and ugly (but working) function:

def find_team(self, name=None, country=None, rank_exactly=None, score_exactly=None, rank_change=None, team_id=None,
                  score_lower=None, score_higher=None, partial_name=None, rank_lower=None, rank_higher=None):
        """
        Finds a team by provided keyword attributes, returns a list of teams matching the filter settings
        Keyword Arguments:
        name - exact team name (case insensitive)
        country - team origin country
        rank_exactly - rank exactly
        rank_lower - only teams with lower rank than provided
        rank_higher - only teams with higher rank than provided
        rank_change - the state of ank change
        score_exactly - score exactly
        score_lower - only teams with lower score than provided
        score_higher - only teams with higher score than provided
        team_id = exact team id
        partial_name - part of name
        """
        if not self.teams:
            raise AttributeError("Object team list is empty")
        results = []
        for team in self.teams:
            all_args=[]
            if name:
                if name.lower() == team.name.lower():
                    all_args.append(True)
                else:
                    all_args.append(False)
            if country:
                if country.lower() == team.country.lower():
                    all_args.append(True)
                else:
                    all_args.append(False)
            if rank_exactly:
                if rank_exactly == team.rank:
                    all_args.append(True)
                else:
                    all_args.append(False)
            if rank_higher:
                if int(rank_higher) > int(team.rank):
                    all_args.append(True)
                else:
                    all_args.append(False)
            if rank_lower:
                if int(rank_lower) < int(team.rank):
                    all_args.append(True)
                else:
                    all_args.append(False)
            if score_exactly:
                if score_exactly == team.score:
                    all_args.append(True)
                else:
                    all_args.append(False)
            if rank_change:
                if rank_change == team.rank_change:
                    all_args.append(True)
                else:
                    all_args.append(False)
            if team_id:
                if team_id == team.team_id:
                    all_args.append(True)
                else:
                    all_args.append(False)
            if score_higher:
                if int(score_higher) < int(team.score.replace(',','')):
                    all_args.append(True)
                else:
                    all_args.append(False)
            if score_lower:
                if int(score_lower) > int(team.score.replace(',','')):
                    all_args.append(True)
                else:
                    all_args.append(False)
            if partial_name:
                if partial_name.lower() in team.name.lower():
                    all_args.append(True)
                else:
                    all_args.append(False)
            if all(all_args):
                results.append(team)
        return results

How could I make this more Pythonic and just better in general?

if name:
            if name.lower() == team.name.lower():
                all_args.append(True)
            else:
                all_args.append(False)
        if country:
            if country.lower() == team.country.lower():
                all_args.append(True)
            else:
                all_args.append(False)
        if rank_exactly:
            if rank_exactly == team.rank:
                all_args.append(True)
            else:
                all_args.append(False)
        if rank_higher:
            if int(rank_higher) > int(team.rank):
                all_args.append(True)
            else:
                all_args.append(False)
        if rank_lower:
            if int(rank_lower) < int(team.rank):
                all_args.append(True)
            else:
                all_args.append(False)
        if score_exactly:
            if score_exactly == team.score:
                all_args.append(True)
            else:
                all_args.append(False)
        if rank_change:
            if rank_change == team.rank_change:
                all_args.append(True)
            else:
                all_args.append(False)
        if team_id:
            if team_id == team.team_id:
                all_args.append(True)
            else:
                all_args.append(False)
        if score_higher:
            if int(score_higher) < int(team.score.replace(',','')):
                all_args.append(True)
            else:
                all_args.append(False)
        if score_lower:
            if int(score_lower) > int(team.score.replace(',','')):
                all_args.append(True)
            else:
                all_args.append(False)
        if partial_name:
            if partial_name.lower() in team.name.lower():
                all_args.append(True)
            else:
                all_args.append(False)

You're being needlessly specific with your if statements here.
If you do something like this...

        if name:
            all_args.append(name.lower() == team.name.lower())
        if country:
             all_args.append(country.lower() == team.country.lower())

and so on, you'd reduce the needless verbosity.


Emitting multiple States for Single Input using ViewModelType architecture Below I have enum representing State of a UITableView.
The states are divided into 3 categories as below.
For all the 3 states I have different UITableViewCells which will be shown for respected cases.
So for fetchMovies input, I am actually checking the movies count and emitting the state & once again after the result.
Can the code written in transform be further reduced or any suggestions.
You can find the architecture here CleanArchitectureRxSwift
import Foundation
import RxSwift
import RxCocoa

protocol ViewModelType {
  associatedtype Input
  associatedtype Output

  func transform(input: Input) -> Output
}

enum MoviesViewModelState {
  case loading
  case loaded([String])
  case loadRemaining
}

class MoviesViewModel: ViewModelType {

  deinit {
    print("HomeViewModel deinit")
  }

  struct Input {
    let fetchMovies: Driver<Void>
  }

  struct Output {
    let movieSection: Driver<MoviesViewModelState>
  }

  var repository: SyncRepository
  private var names: [String] = []
  private let disposeBag = DisposeBag()

  init(repository: SyncRepository) {
    self.repository = repository
  }

  func transform(input: Input) -> Output {
    let movieSection = PublishSubject<MoviesViewModelState>()

    input.fetchMovies
      .asObservable()
      .flatMapLatest { [unowned self] _ -> Observable<[String]> in
        if self.names.count > 0 {
          movieSection.onNext(.loadRemaining)
        } else {
          movieSection.onNext(.loading)
        }
        return self.repository.fetchNames(for: self.names.count) // returns Observable<[String]>
    }.subscribe(onNext: { [unowned self] (names: [String]) in
      self.names.append(contentsOf: names)
      movieSection.onNext(.loaded(self.names))
    }).disposed(by: disposeBag)

    return Output(movieSection: movieSection.asDriverOnErrorJustComplete())
  }
}

extension ObservableType {
  func asDriverOnErrorJustComplete() -> Driver<Element> {
    return asDriver { _ in
      return Driver.empty()
    }
  }
}

Well, the fact that the result of the network request is needed in the next network request means you have a cycle which mean you have to have a subject of some sort. Also, you have to maintain state which implies a BahviorSubject rather than a PublishSubject.
That said, whenever you have an onNext inside a subscribe, there is likely a cleaner way.
class MoviesViewModel: ViewModelType {

    struct Input {
        let fetchMovies: Observable<Void>
    }

    struct Output {
        let movieSection: Driver<MoviesViewModelState>
    }

    private let repository: SyncRepository
    private let disposeBag = DisposeBag()

    init(repository: SyncRepository) {
        self.repository = repository
    }

    func transform(input: Input) -> Output {
        let state = BehaviorSubject<[String]>(value: [])

        let movies = input.fetchMovies
            .withLatestFrom(state)
            .share(replay: 1)

        movies
            .map { $0.count }
            .flatMap { [repository] in repository.fetchNames(for: $0) }
            .withLatestFrom(state) { $1 + $0 }
            .bind(to: state)
            .disposed(by: disposeBag)

        return Output(
            movieSection: Observable.merge(
                movies.map { $0.count == 0 ? .loading : .loadRemaining },
                state.map(MoviesViewModelState.loaded)
            ).asDriverOnErrorJustComplete()
        )
    }
}

One of the interesting things to note is that the BahviorSubject does not need to be a property of the class. The bind will capture the subject for you.


Sudoku solver using backtracking, in Ruby I wrote the below Sudoku solver, using Backtracking. Would love to hear your comments on the approach, and how I can improve readability/performance:

BLANK = '.'

def sudoku(board)
  # find first '.'
  for i in 0...board.length
    for j in 0...board[i].length
      next unless board[i][j] == BLANK

      options = get_options(board, i, j)
      return nil if options.empty? # validity check failed, kill branch

      # put options on board individually, and recurse
      proposed = board.dup
      options.each do |v|
        proposed[i][j] = v
        return proposed if sudoku(proposed)
        proposed[i][j] = BLANK
      end

      # none of the options worked
      return nil
    end
  end

  # base case: the whole board has numbers
  board
end

def get_options(board, row, col)
  # returns the list of potential options for a particular cell based
  # on the game rules:
  # - no repeated numbers in each row
  # - no repeated numbers in a column
  # - no repeated numbers in a block

  options = [*"1".."9"] # all options
  len = board.length

  # remove the options that exist in the same row
  options -= board[row].chars.uniq

  # remove the options that exist in the same column
  column = len.times.collect {|row| board[row][col]}
  options -= column.uniq

  # remove the options that exist in the same block
  block = get_block(board, row, col)
  options -= block.uniq

  # remove blanks and return
  options -= [BLANK]
end

def get_block(board, row, col)
  # returns the entire block that the row/col falls in
  row_start = 3 * (row / 3)
  col_start = 3 * (col / 3)
  block = []

  for i in 0..2
    for j in 0..2
      block << board[row_start + i][col_start + j]
    end
  end
  block
end

board =
   ["53..7....",
    "6..195...",
    ".98....6.",
    "8...6...3",
    "4..8.3..1",
    "7...2...6",
    ".6....28.",
    "...419..5",
    "....8..79"]

puts sudoku(board).inspect

It seems fine to me, I don't know if there is a way to solve this problem that performs better. Some minor things:

The Ruby std-lib has a matrix class which might help. It also might help to extract the board into its own class with methods like dup, get_row, get_column, get_block, is_blank?, is_complete?
You don't need to set proposed[i][j] = BLANK on each iteration since you are working on a duplicate of the board.
In get_options You don't need to call uniq, it should be impossible to get into a state with multiple entries in a row, column or block. Similarly you shouldn't need to remove BLANK as there is no way to get a blank entry in the options array.


Two integers start and end Write a program that reads two integers start and end from the user
  and prints the number from start to end inclusive. However, If end was
  less than start than you should print them in descending order.

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

    int start = scan.nextInt();
    int end = scan.nextInt();

    if(start < end)
        for(int i = start; i <= end; i++)
            System.out.print(i + " ");

    else if(start > end)
        for(int i = start; i >= end; i--)
            System.out.print(i + " ");
}

Input checking

The most obvious issue that I see is that you are not verifying that the correct input is being entered. For example, if you enter a String instead of an int:

> hello
Exception in thread "main" java.util.InputMismatchException
  at java.util.Scanner.throwFor(Scanner.java:864)
  at java.util.Scanner.next(Scanner.java:1485)
  at java.util.Scanner.nextInt(Scanner.java:2117)
  at java.util.Scanner.nextInt(Scanner.java:2076)
  at Main.main(Main.java:8)

A natural way to do this is with Scanner.hasNextInt(). Also, consider adding a little bit of feedback so it is less confusing for the user. Something like this, for example:

    Scanner scan = new Scanner(System.in);
    int start;
    int end;

    System.out.println("Enter 1st whole number: ");
    while (!scan.hasNextInt()) {
        System.out.println("Input must be a whole number. Try again:");
        scan.next();
    }
    start = scan.nextInt();

    System.out.println("Enter 2nd whole number: ");
    while (!scan.hasNextInt()) {
        System.out.println("Input must be a whole number. Try again:");
        scan.next();
    }
    end = scan.nextInt();

Result:

Enter 1st whole number:
> hello
Input must be a whole number. Try again:
> 5
Enter 2nd whole number:
> world
Input must be a whole number. Try again:
> 20
5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20

One other (more general) method (though it is more of an anti-pattern in this particular case) to validate things is with a try/catch block. See this answer on Stack Overflow for an example on how to do this.

    Scanner scan = new Scanner(System.in);
    int start;
    int end;

    try {
        System.out.println("Enter 1st number:");
        start = scan.nextInt();
        System.out.println("Enter 2nd number:");
        end = scan.nextInt();

    } catch (java.util.InputMismatchException e) {
        System.out.println("Input must be an integer.");
        System.out.println("Exiting program.");
        return;
    }

Enter 1st number:
> hello
Input must be an integer.
Exiting program.

Formatting

One other remark, although it is more stylistic, it is often considered a good habit in general to include braces in Java even when they are not needed. So this bit:

if(start < end)
    for(int i = start; i <= end; i++)
        System.out.print(i + " ");

else if(start > end)
    for(int i = start; i >= end; i--)
        System.out.print(i + " ");

Would look like this:

    if(start < end) {
        for(int i = start; i <= end; i++) {
            System.out.print(i + " ");
        }
    }
    else if(start > end) {
        for(int i = start; i >= end; i--) {
            System.out.print(i + " ");
        }
    }


More portable toLower() implementation I am challenging myself to try to attempt to write a function that is as efficient, portable and failsafe as possible. The function is very simple and just converts a string (or wstring) to lowercase.

Are there any improvements you think I could make and any errors I have done?

#ifdef UNICODE
typedef std::wstring tstring;
typedef std::wstring::npos tnpos;
#else
typedef std::string tstring;
//typedef std::string::npos tnpos;  // throws compiler error?!
#endif

tstring toLower(const tstring& str)
{
    // Alternative 1: Returns a copy of a local string so not that efficient.

    unsigned int len = str.length();    // use unsigned int so I only have 4 bytes as opposed to 16 for int
    tstring lower(len, 'a');            // ensure string is correct size to avoid dynamic resizing. Reserve at construction; performing 2 steps at once defintion and reserving - is faster right?
    // or use lower.reserve(len);

    for (unsigned int i=0; i<len; i++)  // iterate using indexes. Maybe iterators could be more fail safe/efficient/etc.?
        lower[i] = tolower(str[i]);

    return lower;
}

void toLower(const tstring& inStr, tstring& outStr)
{
    // Alternative 2: Have user define string and pass by reference (this would be faster?)

    unsigned int len = inStr.length();  // use unsigned int so I only have 4 bytes as opposed to 16 for int. Store length in local variable because we look it up again in the below for loop
    outStr.resize(len);                 // ensure string is correct size to avoid dynamic resizing.

    for (unsigned int i=0; i<len; i++)  // iterate using indexes. Maybe iterators could be more fail safe/efficient/etc.?
        outStr[i] = tolower(inStr[i]);
}

typedef std::wstring::npos tnpos;

std::string::npos and std::wstring::npos are static constant integer members of std::string and std::wstring, respectively. They are not types and they cannot be typedef'd.

unsigned int len = str.length();

This is incorrect. std::string::length() has a return type of size_t std::string::size_type which may be larger than an unsigned int depending on your platform and compiler. Other class methods will use the same type for length/position/indexing, so you should make this change for all the other uses of unsigned int.

// or use lower.reserve(len);

No, please don't. That reserves space for the string, but it does not resize it. There is a resize() method.

Since you said you're able to use C++11 features, as long as they're enabled, the first version of the function (the one that returns a string) will work without copying. It will use the move constructor. So you don't have to use the cumbersome version with an input and output parameter. For more information see http://blog.smartbear.com/c-plus-plus/c11-tutorial-introducing-the-move-constructor-and-the-move-assignment-operator/


Acquiring the number of contracts for persons This script shows the number of contracts for persons. Table Contract has more than 82008 records and Candidate has about 7978.

Could you please suggest improvements for this script?

select ISNULL(CON.ContractCount, 0)
,      CAN.PersoonName
,      ... /* some fields from table Candidate (more than 25) */
from   Candidate CAN
       left join (select PersoonID, COUNT(*) ContractCount
                  from Contract
                  group by PersoonID) CON on CAN.PersoonID = CON .PersoonID
where CAN.Type = 11

Sometimes, when doing left-join aggregation with a simple function (count()), it is easier to move the aggregation to be part of the select clause instead of as a subquery on the from clause.

As long as you are only pulling one aggregate from the sub table, it's easy.

Also, I presume there is at least one index on Contract where the PersoonID is the first (or only) column.

select (select count (*)
        from Contract
        where PersoonID = CAN.PersoonID
       ) as ContractCount
,      CAN.PersoonName
,      ... /* some fields from table Candidate (more than 25) */
from   Candidate CAN
where CAN.Type = 11


Create a background-image over a background-color with different opacities for each element Here the demo

The code below creates a background-image over a background-color with different opacities for each element. Is there a cleaner way of achieving this?

.component{
 min-height: 100vh;
 min-width:100vw;
 font-size:5em;

 display:flex;
 justify-content: center;
 align-items: center;
 color:black;

}
.component::before{
 content: "";

 position: absolute;
 top: 0;
 left: 0;

 width: 100%;
 height: 100%;
 min-height: 100vh;
 min-width:100vw;

 opacity:1;
 z-index:-1;
 background: url("https://www.pngarts.com/files/5/Los-Angeles-Free-PNG-Image.png") center 100px/500px repeat;
}

.component_background_color::after{
 content: "";

 position: absolute;
 top: 0;
 left: 0;

 width: 100%;
 height: 100%;
 min-height: 100vh;
 min-width:100vw;

  opacity:0.3;
 z-index:-2;
 background:green;
}

      Awesome WebPage

You can shorten that code a bit, and then, by using rgba(), you can have both transparent background and image in one pseudo, and using attr() together with the second pseudo, display the text.

The 4th argument in rgba() is the opacity level.

Stack snippet

.component {
 min-height: 100vh;
 min-width:100vw;
 font-size:5em;

 display:flex;
 justify-content: center;
 align-items: center;
 color:black;
}
.component::before {
 content: "";
 position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 height: 100%;
 opacity:1;
 background: rgba(0,255,0,0.3) url("https://www.pngarts.com/files/5/Los-Angeles-Free-PNG-Image.png") center 100px/500px repeat;
}

.component::after {
 content: attr(data-text);
 position: relative;
}

If you intend to use something else inside the component, and to avoid using negative z-index on the pseudo (which can cause other issue when it comes to the stacking context), use e.g. a div and give it position: relative and it will float on top of the pseudo.

Stack snippet

.component {
 min-height: 100vh;
 min-width:100vw;
 font-size:5em;

 display:flex;
 justify-content: center;
 align-items: center;
 color:black;
}
.component::before {
 content: "";
 position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 height: 100%;
 opacity:1;
 background: rgba(0,255,0,0.3) url("https://www.pngarts.com/files/5/Los-Angeles-Free-PNG-Image.png") center 100px/500px repeat;
}

.component div {
 position: relative;
}

    Awesome WebPage

If you want to alter the opacity on the background color and the image individually, you will need both pseudo, and here's how-to.

As the div will become before the ::after pseudo, markup wise, we need to give it z-index: 1 to "float" on top. Note, this is not as bad as using a negative z-index on the pseudo.

Stack snippet

.component {
 min-height: 100vh;
 min-width:100vw;
 font-size:5em;

 display:flex;
 justify-content: center;
 align-items: center;
 color:black;
}
.component::before,
.component::after {
 content: "";
 position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 height: 100%;
 opacity:.3;
 background: green;
}

.component::after {
 opacity:.7;
 background: url("https://www.pngarts.com/files/5/Los-Angeles-Free-PNG-Image.png") center 100px/500px repeat;
}

.component div {
 position: relative;
  z-index: 1;
}

    Awesome WebPage


Statistical calculations with sets of genes The following piece of code executes 20 million times each time the program is called, so I need a way to make this code as optimized as possible.

int WilcoxinRST::work(GeneSet originalGeneSet, vector<string> randomGenes) {
vector<string> geneIDs;
vector<bool> isOriginal;
vector<int> rank;
vector<double> value;
vector<int> score;
int genesPerSet = originalGeneSet.geneCount();
unsigned int totalGenes, tempScore;
/**
 * Fill the first half of the vectors with original gene set data
 */
totalGenes = genesPerSet * 2;
for (int i = 0; i < genesPerSet; i++) {
    geneIDs.push_back(originalGeneSet.getMemberGeneAt(i));
    isOriginal.push_back(true);
    value.push_back(fabs(expressionLevels.getValue(geneIDs[i], statType)));
}
/**
 * Fill the second half with random data
 */
for (unsigned int i = genesPerSet; i < totalGenes; i++) {
    geneIDs.push_back(randomGenes.at(i - genesPerSet));
    isOriginal.push_back(false);
    value.push_back(fabs(expressionLevels.getValue(geneIDs[i], statType)));
}
totalGenes = geneIDs.size();
/**
 * calculate the scores
 */
if (statType == Fold_Change || statType == T_Statistic
        || statType == Z_Statistic) {
    //      Higher value is a winner
    for (unsigned int i = 0; i < totalGenes; i++) {
        tempScore = 0;
        if (!isOriginal[i]) {
            for (int j = 0; j < genesPerSet; j++) {
                if (value.at(i) > value.at(j)) {
                    tempScore++;
                }
            }

        } else {
            for (unsigned int j = genesPerSet; j < totalGenes; j++) {
                if (value.at(i) > value.at(j)) {
                    tempScore++;
                }
            }

        }

        score.push_back(tempScore);
    }

} else if (statType == FDR_PValue || statType == PValue) {
    // Lower value is a winner
    for (unsigned int i = 0; i < totalGenes; i++) {
        tempScore = 0;
        if (!isOriginal[i]) {
            for (int j = 0; j < genesPerSet; j++) {
                if (value.at(i) < value.at(j)) {
                    tempScore++;
                }
            }

        } else {
            for (unsigned int j = genesPerSet; j < totalGenes; j++) {
                if (value.at(i) < value.at(j)) {
                    tempScore++;
                }
            }

        }

        score.push_back(tempScore);
    }

} else {
    cout << endl << "ERROR. Statistic type not defined." << endl;
}

/**
 * calculate Ua, Ub and U
 */
int U_Original = 0, U_Random = 0, U_Final;
for (int j = 0; j < genesPerSet; j++) {
    U_Original += score[j];
}
for (unsigned int j = genesPerSet; j < totalGenes; j++) {
    U_Random += score[j];
}
U_Final = (U_Original < U_Random) ? U_Original : U_Random;

/**
 * calculate z
 */
double Zn, Zd, Z;
Zn = U_Final - ((genesPerSet * genesPerSet) / 2);
Zd = sqrt(
        (double) (((genesPerSet * genesPerSet
                * (genesPerSet + genesPerSet + 1)))) / 12.0);
Z = Zn / Zd;

/**
 * Return 0/1/2
 * 2: p value < 0.01
 * 1: 0.01 < p value < 0.05
 * 0: p value > 0.05
 */
if (fabs(Z) > 2.303)
    return 2;
else if (fabs(Z) > 1.605)
    return 1;
else
    return 0;
}

Your code have a complexity of O(N*N) [genesPerSet=N]. But using the fact that the order of the values is irrelevant for you we can order it in O(N•log(N)) and compute the “scores” in O(N). (With potentially can be thousands time faster).

Also, we have a total of N*N comparisons. Then U_Original + U_Random = N*N, meaning we don’t need to compute U_Random.
Also your statistic Zn= Umin-N*N/2;[Umix=min(U_Original,U_Random)], when you only abs(Zn/Zd) is symmetric around N*N/2. We need only one algorithm.

1.- A first thing can be to take arguments by (const) reference :

int WilcoxinRST::work(const GeneSet &originalGeneSet, const vector<string> &randomGenes)

2.- You fill into vector geneIDs; but dont use it ? Why?

3.- You can iterate only 2 times.

4.- You keep the signal values (probe intensitat?) together in one vector and use another vector to signal what is each item – simple keep in two vector.

5.- You don’t need the score vector, only the summa.

6.- Why 20 millions times? I gess your are computing some “statistic” stability or BStrap. Probably you use the same originalGeneSet many time. I think you can in another question post the code that call this function to spare in making each time the vector of values and sorting.

7.- If you aproximatly know the espected size of the vectors you can reserve() it, add new item with push_back(), and access with .at() (somewhere write a try/catsh). But here we know exactly the numer of item, and we can set it in the cinstructor and then use [] with is the faster way.

Here is first the new O(N•log(N)) code.

What follows is a cleanup of your code but still O(N*N), fast but only by a constant factor.

Then the same code but mixed with yours original code and with more comments.

Please, debugg this and tell me how was.

#include<vector>
#include<algorithm>

int WilcoxinRST::work(const GeneSet &originalGeneSet , const vector<string>& randomGenes)
{
    size_t genesPerSet = originalGeneSet.geneCount();
    std::vector<double> valueOri(genesPerSet), valueRnd(genesPerSet);
    /**
     * Fill the valueOri vector with original gene set data, and valueRnd with random data
     */
    for (size_t i = 0; i < genesPerSet; i++)
    {
      valueOri[i] = std::fabs(expressionLevels.getValue( originalGeneSet.getMemberGeneAt(i) , statType ));
      valueRnd[i] = std::fabs(expressionLevels.getValue( randomGenes.at(i)                  , statType ));
    }
    std::sort(valueOri.begin(),valueOri.end());
    std::sort(valueRnd.begin(),valueRnd.end());

    /**
     * calculate the scores Ua, Ub and U
     */
    long U_Original=0 ;

    if (statType == Fold_Change || statType == T_Statistic  || statType == Z_Statistic
        statType == FDR_PValue  || statType == PValue )
    {
        //      Higher value is a winner
        size_t j=0;
        for (size_t i = 0; i < genesPerSet /*totalGenes*/; i++)      // i   -  2x
        {
            while(valueOri[i]  > valueRnd[j]) ++j ;
            U_Original += j;
        }
    } else { cout << endl << "ERROR. Statistic type not defined." << endl;  }

    /**
     * calculate z
     */
    double Zn, Zd, Z;
    Zn = U_Original - ((genesPerSet * genesPerSet) / 2);
    Zd = std::sqrt( (double) (((genesPerSet * genesPerSet* (genesPerSet + genesPerSet + 1)))) / 12.0);
    Z = Zn / Zd;

    /**
     * Return 0/1/2
     * 2: p value < 0.01
     * 1: 0.01 < p value < 0.05
     * 0: p value > 0.05
     */
         if (std::fabs(Z) > 2.303)  return 2;
    else if (std::fabs(Z) > 1.605)  return 1;
    else                            return 0;
}

What follows is a cleanup of your code but still O(N*N), fast but only by a constant factor.

#include<vector>
using namespace std;
class GeneSet ;
class WilcoxinRST;

int WilcoxinRST::work(const GeneSet &originalGeneSet , const vector<string>& randomGenes)
{
    size_t genesPerSet = originalGeneSet.geneCount();
    vector<double> valueOri(genesPerSet), valueRnd(genesPerSet);
    /**
     * Fill the valueOri vector with original gene set data, and valueRnd with random data
     */
    for (size_t i = 0; i < genesPerSet; i++)
    {
        valueOri[i]   =  fabs(expressionLevels.getValue( originalGeneSet.getMemberGeneAt(i) , statType ));
        valueRnd[i]   =  fabs(expressionLevels.getValue( randomGenes.at(i)                  , statType ));
    }
    /**
     * calculate the scores Ua, Ub and U
     */
    long U_Original = 0, U_Random = 0, U_Final;

    if (statType == Fold_Change || statType == T_Statistic  || statType == Z_Statistic)
    {
        //      Higher value is a winner
        for (size_t i = 0; i < genesPerSet /*totalGenes*/; i++)      // i   -  2x
        {   for (size_t j = 0; j < genesPerSet; j++)
            {   U_Random  +=  (valueRnd[i]  > valueOri[j]);// i en 2 set=Rnd, j in 1 set=Ori. count how many Ori are less than this Rnd
                U_Original+=  (valueOri[i]  > valueRnd[j]);// i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are less than this Ori
            }
        }
    } else
    if (statType == FDR_PValue || statType == PValue)
    {
        // Lower value is a winner
        for (size_t i = 0; i < genesPerSet; i++)
        {
            for (size_t j = 0; j < genesPerSet; j++)
            {   U_Random  +=  (valueRnd[i]  < valueOri[j]);// i en 2 set=Rnd, j in 1 set=Ori. count how many Ori are > than this Rnd
                U_Original+=  (valueOri[i]  < valueRnd[j]);// i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are > than this Ori
            }
        }
    } else { cout << endl << "ERROR. Statistic type not defined." << endl;  }

    U_Final = (U_Original < U_Random) ? U_Original : U_Random;

    /**
     * calculate z
     */
    double Zn, Zd, Z;
    Zn = U_Final - ((genesPerSet * genesPerSet) / 2);
    Zd = sqrt(
            (double) (((genesPerSet * genesPerSet
                    * (genesPerSet + genesPerSet + 1)))) / 12.0);
    Z = Zn / Zd;

    /**
     * Return 0/1/2
     * 2: p value < 0.01
     * 1: 0.01 < p value < 0.05
     * 0: p value > 0.05
     */
         if (fabs(Z) > 2.303)       return 2;
    else if (fabs(Z) > 1.605)       return 1;
    else                            return 0;
}

the same code but mixed with yours original code and with more comments.

int WilcoxinRST::work(const GeneSet &originalGeneSet , const vector<string>& randomGenes)
{
    size_t genesPerSet = originalGeneSet.geneCount();
    unsigned int totalGenes, tempScore;
    totalGenes = genesPerSet * 2;

    //vector<string> geneIDs;
    //vector<bool>   isOriginal;
    //vector<int>    rank;
    vector<double> valueOri(genesPerSet), valueRnd(genesPerSet);
    //vector<int>    score;
    /**
     * Fill the first half of the vectors with original gene set data
     */

    for (size_t i = 0; i < genesPerSet; i++)
    {
        //geneIDs.push_back( originalGeneSet.getMemberGeneAt(i)  );
        //isOriginal.push_back(true);

        valueOri[i]   =  fabs(expressionLevels.getValue( originalGeneSet.getMemberGeneAt(i) , statType ));
        valueRnd[i]   =  fabs(expressionLevels.getValue( randomGenes.at(i)                  , statType ));
    }
    /**
     * Fill the second half with random data
     */
    //for (unsigned int i = genesPerSet; i < totalGenes; i++) {
    //  geneIDs.push_back(randomGenes.at(i - genesPerSet));
    //  isOriginal.push_back(false);
    //  value.push_back(fabs(expressionLevels.getValue(geneIDs[i], statType)));
    //}
    //totalGenes = geneIDs.size();
    /**
     * calculate the scores
     */
        /**
     * calculate Ua, Ub and U
     */
    long U_Original = 0, U_Random = 0, U_Final;
    //for (int j = 0; j < genesPerSet; j++) // j in 1 set=Ori. count how many Ori are less than this Rnd
    //{
    //  U_Original += score[j];
    //}
    //for (unsigned int j = genesPerSet; j < totalGenes; j++) // j in 2 set=Rnd, count how many Rnd are less than this Ori
    //{
    //  U_Random += score[j];
    //}

    if (statType == Fold_Change || statType == T_Statistic  || statType == Z_Statistic)
    {
        //      Higher value is a winner
        for (size_t i = 0; i < genesPerSet /*totalGenes*/; i++)      // i   -  2x
        {   //tempScore = 0;
            //if (!isOriginal[i])  // i en 2 set=Rnd, j in 1 set=Ori. count how many Ori are less than this Rnd
            for (size_t j = 0; j < genesPerSet; j++)
            {   U_Random  +=  (valueRnd[i]  > valueOri[j]);// i en 2 set=Rnd, j in 1 set=Ori. count how many Ori are less than this Rnd
                U_Original+=  (valueOri[i]  > valueRnd[j]);// i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are less than this Ori
            }
            //} else
            //{
            //  for (unsigned int j = genesPerSet; j < totalGenes; j++)  // i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are less than this Ori
            //  {   if (value.at(i) > value.at(j)) {    tempScore++;        }
            //  }

            //}
            //score.push_back(tempScore);
        }

    } else
    if (statType == FDR_PValue || statType == PValue)
    {
        // Lower value is a winner
        for (size_t i = 0; i < genesPerSet; i++)
        {
            for (size_t j = 0; j < genesPerSet; j++)
            {   U_Random  +=  (valueRnd[i]  < valueOri[j]);// i en 2 set=Rnd, j in 1 set=Ori. count how many Ori are > than this Rnd
                U_Original+=  (valueOri[i]  < valueRnd[j]);// i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are > than this Ori
            }
            //} else
            //{
            //  for (unsigned int j = genesPerSet; j < totalGenes; j++)  // i in 1 set=Ori, j in 2 set=Rnd, count how many Rnd are less than this Ori
            //  {   if (value.at(i) > value.at(j)) {    tempScore++;        }
            //  }

            //}
            //score.push_back(tempScore);
        }

        //for (unsigned int i = 0; i < totalGenes; i++)
        //{   tempScore = 0;
        //  if (!isOriginal[i])
        //  {   for (int j = 0; j < genesPerSet; j++) {
        //          if (value.at(i) < value.at(j)) {  // Rnd i < Ori j increm U_Random
        //              tempScore++;
        //          }
        //      }

        //  } else {
        //      for (unsigned int j = genesPerSet; j < totalGenes; j++) { // Ori i < Rnd j. Increm U_Original
        //          if (value.at(i) < value.at(j)) {
        //              tempScore++;
        //          }
        //      }

        //  }

        //  score.push_back(tempScore);
        //}

    } else { cout << endl << "ERROR. Statistic type not defined." << endl;  }

    U_Final = (U_Original < U_Random) ? U_Original : U_Random;

    /**
     * calculate z
     */
    double Zn, Zd, Z;
    Zn = U_Final - ((genesPerSet * genesPerSet) / 2);
    Zd = sqrt(
            (double) (((genesPerSet * genesPerSet
                    * (genesPerSet + genesPerSet + 1)))) / 12.0);
    Z = Zn / Zd;

    /**
     * Return 0/1/2
     * 2: p value < 0.01
     * 1: 0.01 < p value < 0.05
     * 0: p value > 0.05
     */
    if (fabs(Z) > 2.303)
        return 2;
    else if (fabs(Z) > 1.605)
        return 1;
    else
        return 0;
}


c# QuadTree yielding properly I have implemented a QuadTree of my own, and I'm afraid I didn't use yield properly when I query my tree, my fear is that I create O(HN) iterators.

Could you direct me how to better improve the performance of the following code?

QuadTree

public IEnumerable<T> Query(string leafId, Circle shape)
    {
        // Gets the relevant parent by a leafId Query (this is O(lg H))
        var startNode = GetParentIntersecting(leafId, shape);

        return startNode.Query(shape);
    }

QuadTreeNode

 public IEnumerable<T> Query(IShape shape)
    {
        var results = new HashSet<T>();

        foreach (var content in _contents)
        {
            if (shape.Contains(content.GetPosition()))
            {
                //Debug.Log(content.GetPosition());
                yield return content;
                //results.Add(content);
            }
        }

        if (IsLeaf)
            yield break;

        foreach (var node in Nodes)
        {
            if (node.IsEmpty && node.IsLeaf)
                continue;

            foreach (var result in node.Query(shape))
            {
                results.Add(result);
            };
        }
    }

You use yield properly, it's OK to yield from inside nested loops. But all the code behind your first foreach is kind of useless, it's not doing anything useful.

First the yield break is useless, cause there are no more yields following, there are no more return values produced no matter if IsLeaf true or false. Iteration ends anyway.

Second you fill up results with something, but the content of results is lost, then the Method exits.


Call multiple services from angular2 component This is my first Angular2 component, which makes two api calls: the first call  provide the input for the the second one.

The following is my attempt at this code.

@Component({
      selector: 'result-panel',
      templateUrl: 'panel.component.html',
      styleUrls: ['panel.component.css'],
      providers:[FindIpZoneDataService,ApiService]
    })

    export class PanelComponent
    {
      networkDetail:Array<Network>;
      query = "";
      datafetchservice:any;
      searching:boolean = false;
      rowCount:boolean = false;
      iperror :boolean = false;
      nwerror :boolean = false;
      userinputerror:boolean = false;
      constructor(private dataservice :FindIpZoneDataService){
            this.datafetchservice = dataservice;
      }

      getNetworkDetail(ip:string) {
        this.networkDetail = [];
        this.dataservice.getNetworkDetail(ip)
          .map((response) => {
            let res: Array<any> = [];
            let extattrs: Array<any> = [];
            res = response.json();
            res.forEach((detail) => {
              extattrs.push(detail.extattrs);
            });
            return extattrs;
          })
          .map((network:Array<any>) => {
            console.log(network);
            let result: Array<Network> = [];
            if (network) {
              network.forEach((detail) => {
                result.push(new Network(detail['SITE Name'].value,
                  detail['InfoSec Security Zone'].value,
                  detail['Network Security Zone'].value,
                  detail['Data Classification'].value));
              });
              return result;
            }
          })
          .subscribe(details => {
              this.networkDetail = details;
              this.rowCount = true;
              this.searching = false;
            },
            err => {
              //Valid Network not found
              this.handleNwServiceError(err);
            });
    }
      getIPNetwork(){
        this.datafetchservice.getNetwork(this.query)
          .map(response => response.json())
          .subscribe(result => {
              this.getNetworkDetail(result[0].network);
          },
            err => {
              //InValid IP
              this.handleIpServiceError(err);
            });
      }

      private handleNwServiceError(error: any) {
        //Observable.throw(error.json().error || 'Invalid Network - Server error');
        console.log(error);
        this.nwerror = true;
        this.rowCount = false;
      }

      private handleIpServiceError(error: any) {
        //Observable.throw(error.json().error || 'Invalid IP - Server error');
        console.log(error);
        this.iperror = true;
        this.rowCount = false;
        console.log('ip error');
      }

Please let me know how I can improve it.

Specify types explicitly whenever it's possible (including void for methods and FindIpZoneDataService for datafetchservice instead of any).
Use const instead of let -- it will help detecting unintended assignments.
Try not to move things around when it's not necessary. E.g. the following section is pushing one array's items into another.

let res: Array<any> = [];
let extattrs: Array<any> = [];
res = response.json();
res.forEach((detail) => {
  extattrs.push(detail.extattrs);
});
return extattrs;

Why do that if simple cast would work? Like this: <T[]>response.json() where <T> is whatever the specific type of the entity returned by the API.

Stylistically, it's better to declare getIPNetwork() before getNetworkDetail() since the code is normally being read top-to-bottom.
When one map() is immediately followed by another map(), they can always be merged into a single one. There is possibility of using flatMap() in addition to that, which would simplify some things but I need better understanding of your context...

getIPNetwork(): void {
    this.datafetchservice
        .getNetwork(this.query)
        .map(response => response.json())
        .subscribe(
            result => this.getNetworkDetail(result[0].network),
            err => this.handleIpServiceError(err) // InValid IP
        );
}

getNetworkDetail(ip: string): void {
    this.networkDetail = [];
    this.dataservice
        .getNetworkDetail(ip)
        .map((response) => {
            return response
                .json()
                .map(detail => new Network(
                    detail['SITE Name'].value,
                    detail['InfoSec Security Zone'].value,
                    detail['Network Security Zone'].value,
                    detail['Data Classification'].value
                ));
        })
        .subscribe(
            details => {
                this.networkDetail = details;
                this.rowCount = true;
                this.searching = false;
            },
            err => this.handleNwServiceError(err) // Valid Network not found
        );
}

extattrs, nwerror, and similar names are meaningless/hard to decipher.
TypeScript just like JavaScript follows camelCase naming convention. Stick to it. I.e. userinputerror should be userInputError, datafetchservice -- dataFetchService, and so on...


Slider control constructor I've made myself a JavaScript slider constructor. You can create new sliders and tell it what elements it should refer to.

var SliderControl = function (container, area, fill, action, actionWhenActive, actionWhenSet) {
    'use strict';
    var self = this;
    this.action = action;
    this.container = container;
    this.area = area;
    this.fill = fill;
    this.element = container.find(area);
    this.value = 0;
    this.active = false;
    this.actionWhenActive = actionWhenActive;
    this.actionWhenSet = actionWhenSet;
    $(document).on('mousemove', function (event) {
        self.move(event, self);
    });
    $(document).on('mouseup', function (event) {
        self.drop(event, self);
    });
    this.area.on('mousedown', function (event) {
        self.grab(event, self);
    });
};

SliderControl.prototype = {
    action: this.action,
    width: function () {
        'use strict';
        var calcWidth = ((this.value * 100) + '%');
        this.fill.width(calcWidth);
    },
    update: function (event, self) {
        'use strict';
        if (this.actionWhenActive === true) {
            this.action();
        }
        var direction, percent, container, area;
        direction = event.pageX - this.element.offset().left;
        percent = Math.min(Math.max(direction / this.element.width(), 0), 1.0);
        this.value = percent;
        this.width();
    },
    move: function (event, self) {
        'use strict';
        if (this.active === true) {
            this.update(event);
        }
    },
    grab: function (event, self) {
        'use strict';
        this.active = true;
        self.update(event);
        event.preventDefault();
    },
    drop: function (event, self) {
        'use strict';
        if (this.active === true) {
            this.active = false;
            this.action();
        }
    },
    setValue: function (value) {
        if (this.active === false) {
            this.value = value;
            this.width();
            if (this.actionWhenSet === true) {
                this.action();
            }
        }
    }
};

If you want to try it out, go here.

I think there's a better way to set a custom function it should run when you drop the slider (or if actionWhenActive is set to true, while you're dragging it) but I'm not sure what would be the most DRY way to do it.

Really your question boils down to whether or not there is a better way to set a custom function. That's hard to say because your approach will work fine. Really any kind of answer your going to get is largely subjective.

For me I tend to avoid the prototype method for objects unless I'm overriding or extending some other function or object. You started on the right track with your declaration of the SliderControl object; you can incorporate your prototype into that method declaration as well to greatly simplify and encapsulate your code.

It could use some cleanup I'm sure but here's my quick take.

<!DOCTYPE>

  <title>Slider</title>
  <meta charset="utf-8" />

The value is 0.4

Now above and beyond your initial question...

To make your slider more portable (and practical) you should also create the dom structure in the object so that all you have to pass in during initialization is the element you'd like to put it in.

You'll also want to add the ability to set a default value, in case you're loading and editing preset data.

I would also suggest removing 'actionWhenActive' and 'actionWhenSet' and replacing them with your existing 'action' and a new 'actionSet' function. You can check if one or the other is null or undefined to give you the same effect as the booleans. This will also give your slider the ability to perform both operations (ie displaying a value, and then saving it when done).


Extract list of selected contractors by matching serialized array in Rails I'm working on an application where I have a Company model setup as follows:

class Company < ActiveRecord::Base
  has_many :employee_roles
  has_many :employees
  validates :company_name, uniqueness: true
  serialize :contractors, Array

end

Contractors is related to the company as a serialized array and saves the data in the db as ["2", "1"] where 2 and 1 are the id's of the contractors.

My code to extract the list of selected contractors from this is:

def coach_list
  @contractors = Contractor.where({ verified: true,
                        validated: true })
              .where('membership_type != ?', Contractor.membership_types["no_membership"])
  @company = Company.find(contractor_company_params[:company_id])
  @selected_contractors = @company.contractors.map { |c| Contractor.find(c.to_i) }
  @unselected_contractors = @contractors - @selected_contractors
end

I'm not sure if this is the best way to extracted @selected_contractors and @unselected_contractors

If your database supports array types natively I would use that  feature, I know that Postgres does. I would also call the field contractor_ids or even selected_contractor_ids then use a method called contractors or selected_contractors to return the actual objects.

I would change this:

@contractors = Contractor.where({ verified: true, validated: true })
                         .where('membership_type != ?', Contractor.membership_types["no_membership"])

to a scope or scopes on the Contractor class. You can also code the second where clause as where.not(membership_type: Contractor.membership_types["no_membership"])

And I would change this:

@selected_contractors = @company.contractors.map { |c| Contractor.find(c.to_i) }

to be a method on the Company class. I would also write it as:

@selected_contractors = Contractor.find(@company.contractors)

this would issue a single query instead of n queries, additionally the to_i is not usually necessary.


Convert reader to list of class Class Student
{
   Int studentId,
   List<ClassB> Books,
  Bool IsPassed
}
Class Book
{
 Int bookId,
 List<int> pages
 }

I am creating a list of students the way below

    static void Main(string[] args)
     {

    var  students = GetStudents();
    var result = students.GroupBy(r => new { r.StudentId, r.IsPassed })
.Select(c =>
new Student()
{
IsPassed = c.Key.IsPassed,
StudentId = c.Key.StudentId,
Books = c.SelectMany(t=>t.Books).ToList()
}).ToList();

    }

private static List<Student> GetStudents() {

string connStr = ConfigurationManager.AppSettings["Conn"];

List<Student> students = new List<Student>(); SqlConnection conn = new SqlConnection(connStr); conn.Open();
SqlCommand command = new SqlCommand("Select * from Students", conn);

using (SqlDataReader reader = command.ExecuteReader())
{
while (reader.Read())
 {
 var StudentId =             Convert.ToInt32(reader["StudentId"]); var BookId = Convert.ToInt32(reader["BookId"]); var IsPassed = Convert.ToBoolean(reader["IsPassed"]); var PageNUmbers = reader["PageNumber"].ToString().Split(',').Select(t => Convert.ToInt32(t)).ToList(); List<Book> books = new List<Book>(); books.Add(new Book() { BookId = BookId, PageNumber = PageNUmbers });
students.Add(new Student()
{
StudentId = StudentId,
Books = books,
 IsPassed = IsPassed
 });

 } }
return students;

}

Is there any better way to doo this?

Some things that just pop out when seeing your code:

1) Classes/POCOs definition

They do not seem properly written, probably reproduced from memory. A better version would be:

class Student
{
    int StudentId { get; set; }
    List<Book> Books { get; set; }
    bool IsPassed { get; set; }
}

class Book
{
    int BookId { get; set; }
    List<int> Pages { get; set; }
}

I have used properties, a more generic list type (IList<>) and put a more C#ish capitalization (Pascal case).

2) Proper disposal of disposable objects

SqlConnection implements IDisposable and should also included in a using block (as its close friends SqlCommand and SqlDataReader). Also, in order to shorten things a bit, C# allows usage of var to replace the actual data type:

private static List<Student> GetStudents()
{
    string connStr = ConfigurationManager.AppSettings["Conn"];

    var students = new List<Student>();
    using (var conn = new SqlConnection(connStr))
    {
        conn.Open();
        var command = new SqlCommand("Select * from Students", conn);

        using (var reader = command.ExecuteReader())
        {
            while (reader.Read())
            {
                var StudentId = Convert.ToInt32(reader["StudentId"]);
                var BookId = Convert.ToInt32(reader["BookId"]);
                var IsPassed = Convert.ToBoolean(reader["IsPassed"]);
                var PageNUmbers = reader["PageNumber"].ToString()
                    .Split(',')
                    .Select(t => Convert.ToInt32(t))
                    .ToList();

                var books = new List<Book>();
                books.Add(new Book() { BookId = BookId, PageNumber = PageNUmbers });
                students.Add(new Student()
                {
                    StudentId = StudentId,
                    Books = books,
                    IsPassed = IsPassed
                });
            }
        }
    }

    return students;
}

2) * usage in SQL scripts

It is recommended to avoid * usage in SQL scripts and provide the actual columns. If columns are added to the table and your code does not need them, their selection is useless.

3) Unhandled exceptions

Your code does not handle any of the numerous exceptions that might arise from the operations (I will not go into the actual exception types, but illustrate in natural words possible problems)

conn.Open(); --> connection might failed due to incorrect connection string or server not available
var IsPassed = Convert.ToBoolean(reader["IsPassed"]); --> conversion failure if IsPassed is not convertible to boolean
.Select(t => Convert.ToInt32(t)) --> fails if at least one split token is not an integer

[later edit]

It is recommended to catch exception and also log the detailed information somewhere. Ideally, end user should receive a friendly message and context details should be logged in a file, database etc. Context details may include username, IP, hostname, call stack.

For .NET, one of the most used libraries is NLog, which is quite easy to setup and use.

e.g. conn.Open() may have the fail for two main reasons that have different exception types:

try
{
    conn.Open();
}
catch (InvalidOperationException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connection to get students");
    // rethrow because there is nothing else to do without connection. All call stack should remain intact
    // caller must be able to handle InvalidOperationException exception type
    throw;
}
catch (SqlException exc)
{
    logger.Log(LogLevel.Error, exc, "Failed to open connections to get students due to authentication problems");
}

4) Separation of concerns

It is better to separate data connection setup from your data fetch, as setup can also be used for other operations in the future. Also, methods should have meaningful names (e.g. GetStudents -> GetAllStudentData)

// greatly simplified - usually, a connection could be reused to avoid reopening it
class DataAccess
{
    private string connStr = ConfigurationManager.AppSettings["Conn"];

    public SqlConnection GetConnection()
    {
        var conn = new SqlConnection(connStr));
        conn.Open();
        return connection;
    }
}

class StudentService
{
    DataAccess dataAccess { get; private set; }

    public StudentService()
    {
        dataAccess = new DataAccess();
    }

    // this may be put private, if its exposure does not make sense
    public List<Student> GetAllStudentData()
    {
        using (var conn = dataAccess.GetConnection())
        {
            // data fetch comes here
        }
    }

    public IList<Student> GetStudentsGroupedByPassed()
    {
        var students = GetAllStudentData();

        groupedStudents = students.GroupBy(r => new { r.StudentId, r.IsPassed })
            .Select(c =>
                new Student()
                {
                    IsPassed = c.Key.IsPassed,
                    StudentId = c.Key.StudentId,
                    Books = c.SelectMany(t=>t.Books).ToList()
                }).ToList();
        }

        return groupedStudents;
    }
}

5) Try to use mainstream frameworks such as EntityFramework to handle data fetch dirty work. Fetch would look like this:

var students = DataContext.Student.Select(s =>
    new Student()
        {
            StudentId = s.StudentId,
            Books = new List<Book>()
                {
                    new Book()
                        {
                            BookId = s.BookId,
                            PageNumber = s.PageNumber.ToString().Split(',')
                                .Select(t => Convert.ToInt32(t))
                                .ToList(),
                            IsPassed = s.IsPassed
                        }
                }
        }.ToList();