How to replace the given email address with a value in a given input? I have written the below method to replace some of the email domains like @gmail.com and @yahoo.com with a given text.
public static string RemovePersonalInfo(string input)
{
    string[] tokens = input.Split(new char[] { ' ', '\t', '\r', '\n' });
    string output = string.Empty;
    foreach (string token in tokens)
    {
        if (token.Contains("@gmail.com"))
        {
            output += " SOMETEXT";
        }
        else
        {
            output += " " + token;
        }
    }
    tokens = output.Split(new char[] { ' ', '\t', '\r', '\n' });
    output = string.Empty;
    foreach (string token in tokens)
    {
        if (token.Contains("@yahoo.com"))
        {
            output += " SOMETEXT";
        }
        else
        {
            output += " " + token;
        }
    }
    return output;
}

It is working as expected for the below input.
But I don't think it is a good solution, I can see the improvements in the code like it is not scalable, let's see tomorrow some other email domain comes, I will have to again modify the code and write another if condition. the second improvement is that I am running the loop twice, it can be done in one loop. so performance can be improved.
Or if there is any better approach than this, please suggest.
Input: test@gmail.com test@abc.com @teest@yahoo.com
Output:   SOMETEXT test@abc.com SOMETEXT
Note: I am not supposed to use the Replace method. So the only intention here is to use the same logic in basic programming languages like C and C++ as well.

Performance wise using string concatination (output += ) inside a loop isn't the best choice because for each output +=  a new string is created because strings are immutable. Using a StringBuilder would be the way to go. But instead of returning a string, you could return an IEnumerable<string> which would make the code more flexible.

Passing an IEnumerable<string> as method parameter holding the not wanted domains will make the code open for needed changes.

Passing an IEnumerable<string> as method parameter holding the tokens would enable the method to only do one thing.

Because the method is public you should have proper parameter validation.

Using a little bit of Linq would compact the code a lot.

Instead of hardcoding "SOMETEXT" you could pass an optional parameter.

Implementing the mentioned changes using string as return-type would lead to
public static string RemovePersonalInfo(string input)
{
    if (input == null) { throw new NullReferenceException(nameof(input)); }
    if (string.IsNullOrWhiteSpace(input)) { return input; }

    return RemovePersonalInfo(input.Split(new char[] { ' ', '\t', '\r', '\n' }), new string[] { "@gmail.com", "@yahoo.com" });

}
private static string RemovePersonalInfo(IEnumerable<string> tokens, IEnumerable<string> domains, string replacement = "SOMETEXT")
{
    return string.Join(" ", tokens.Select(token => (domains.Any(domain => token.Contains(domain)) ? replacement : token)));
}

and could be called like before.


Am I violating any sound coding principles with this approach? I am creating a C# service that will be doing some specialized processing of data coming into our ODS (Operational Data Store) DB. To support this, I’m building a DAL that will be used to access our ODS. In my DAL, I’ll have a number of GetItem (get a single item) and GetList (get a list of items) methods with a number of overrides to support a number of combinations of parameters for each method.

I was think that instead of having a number of overrides, I would do the following, using GetList as an example, using an Expression as the only parameter:

/// <summary>
/// Gets a list of Marker entities based on a predicate.
/// </summary>
/// <param name="predicate">The predicate</param>
/// <returns>List of Marker entities</returns>
public List<Entities.Marker> GetList(Expression<Func<MarkerRecord, bool>> predicate)
{
    return this.Database.Marker.Where(predicate).Map();
}

And it would be called like this:

MarkerData markerData = new MarkerData();
var markerList = markerData.GetList(row => row.ReadTime >= new DateTime(2012, 1, 7));

I could even modify this to make it generic to handle any data type. While this solution offers flexibility and saves some coding, something tells me I'm violating one or more best practices, perhaps like separation of concerns by passing code as data. Do you see any problems with this approach?

As you've discovered complex DALs are ugly to build because they are just an uninteresting hard to maintain bunch of plumbing :(

Fortunately some smart guys have built generics DALs that you can leverage to do almost all the operations like projecting and filtering you want.

When possible they will be processed on the server side which can be a huge gain in performance because you don't have to pass enormous quantity of data around, just the result you want to further process or display to the user.

Have a look at NHibernate which offers the kind of stuff you're building plus many more.

In my opinion he is the best on the market but there is some interesting challengers like Entity framework from the fourth version.

And before asking yourself if you're violating any good practices, principle or dogma, rather ask if it could cause any issue in term of code quality (readability, maintainability, robustness...).

We other geeks are too focused on what is best rather than on what should be done now, it personally took me (a lot of) years, and some experience in front office development, to realize that ... but I'm not completely cured. ;)


Books Algorithm Dealing with Square Digit Sums A little context:

  Book has 411 Pages
  Read a random number of pages on the first day which is unknown
  Number of pages to read next day is the square of the sum of the digits of the page currently on

  I.e. on Page 36: read 81 pages next day
  On day 6 The square answer directly adds to current page number to make 411
  Finish Book

  How many pages did she read on the first day and how many pages did
  she read on the 6 days?

This is what I have so far:

from itertools import accumulate, count

def read_book( start ):
    for i in range( 6 ): yield start; start += sum( int( digit ) for digit in str( start ) ) ** 2

def find_start( pages ):
    for start in count( 1 ):
        for read in read_book( start ): pass
        if read == pages: return start

previous_answer, start = 0, find_start( 411 )
for i, read in enumerate( read_book( start ), 1 ):
    print( "Pages read on day", i ,"was", read-previous_answer ,"\t(Now on page", str(read) + ")\n")
    previous_answer += read-previous_answer

Output:

  Pages read on day 1 was 61 (Now on page 61)

  Pages read on day 2 was 49 (Now on page 110)

  Pages read on day 3 was 4 (Now on page 114)

  Pages read on day 4 was 36 (Now on page 150)

  Pages read on day 5 was 36 (Now on page 186)

  Pages read on day 6 was 225 (Now on page 411)

Is there a more efficient way of writing this using fewer lines of code?

Can someone see if there is a more efficient way of writing this using fewer lines of code?

I suppose your focus is not literally on the part about "fewer lines of code".
You could use more semicolons to squeeze more lines onto a single line,
but that would violate PEP 20, the Zen of Python,
even more than it already does.

Your current solution is essentially a brute force algorithm,
counting up from the beginning until you find a suitable start page from which the last page will be reached on Day 6.
The algorithm is well implemented,
and I don't know a better way.
I can suggest some other improvements though.

Coding style

You should follow PEP8, the Python coding style guide.
In particular:

Do break lines in loops
Avoid using semicolon to separate multiple statements
Don't put spaces around method parameters, for example write read_book(start) instead of read_book( start )

Generalize

It would be nice to make the implementation more general.

For example, the target pages 411 and the required number of days would be nice to put in constants, to make them easier to play with and less magic.

Also,
instead of executing code in the global namespace,
the best practice is wrap your main code in a main() method,
and call that from within if __name__ == '__main__':, like this:

def main():
    # main implementation

if __name__ == '__main__':
    main()

Robustness

The find_start method counts up from 1 until the target page is reached,
or else...forever.
It would better to set an upper limit, like this:

def find_start(pages):
    for start in range(1, pages):
        read = start
        for read in read_book(start):
            pass
        if read == pages:
            return start
    raise ValueError("Impossible to reach page {} exactly".format(pages))

Suggested implementation

Putting the above suggestions together:

PAGES = 411
DAYS_TO_READ = 6

def read_book(start):
    for _ in range(DAYS_TO_READ):
        yield start
        start += sum(int(digit) for digit in str(start)) ** 2

def find_start(pages):
    for start in range(1, pages):
        read = start
        for read in read_book(start):
            pass
        if read == pages:
            return start
    raise ValueError("Impossible to reach page {} exactly".format(pages))

def main():
    previous_answer, start = 0, find_start(PAGES)
    for i, read in enumerate(read_book(start), 1):
        print("Pages read on day {} was {}\t(Now on page {})\n".format(
            i, read - previous_answer, read))
        previous_answer += read - previous_answer

if __name__ == '__main__':
    main()


Is there a more readable way to write this add to a list only if the element not exists? (perfectionistic) Take a timer in your hands, then have a look to the following code. What is it doing?
I took 52 seconds to realize more or less what it does..
It compares a list called categories with an actual element. If it already exist in the list it takes it and stores to the category variable. Otherwise it creates this new element, stores it into the category variable and also add it to the list.

Mmm... very not clear to me. I strongly prefer the second piece of code.
What do you think? How would you rewrite the code to make it more readable?
Do you find an utility in generalizing patterns like this one and creating classes and framework for handling this behaviors in a standardized way?

foreach (var summaryOdd in odds)
{
    var category = new PlacedCategory();
    var eventId = summaryOdd.Championship;

    if (categories.Any(cat => cat.Name == eventId))
    {
        category = categories.First(cat => cat.Name == eventId);
    }
    else
    {
        category.Name = eventId;
        categories.Add(category);
    }

foreach (var summaryOdd in odds)
{
    var category = new PlacedCategory();
    var eventId = summaryOdd.Championship;

    category = FindOrCreateCategoryByEventId(categories, eventId);

I don't think it is necessary to create a method; the code seems atomic enough (SRP).

But I would refactor the code to make it clearer, and to avoid enumerating the categories twice:

foreach (var summaryOdd in odds)
{
    string eventId = summaryOdd.Championship;
    var category = categories.FirstOrDefault(cat => cat.Name == eventId);
    if(category == null)
    {
        category = new PlacedCategory
        {
            Name = eventId,
        };
        categories.Add(category);
    }
}


Finding a vertex of a tree such that every subtree rooted at that vertex is of a uniform color I'm trying to optimally solve codeforces problem 763A:

  Each New Year Timofey and his friends cut down a tree of n vertices
  and bring it home. After that they paint all the n its vertices, so
  that the i-th vertex gets color ci.

  Now it's time for Timofey birthday, and his mother asked him to remove
  the tree. Timofey removes the tree in the following way: he takes some
  vertex in hands, while all the other vertices move down so that the
  tree becomes rooted at the chosen vertex. After that Timofey brings
  the tree to a trash can.

  Timofey doesn't like it when many colors are mixing together. A
  subtree annoys him if there are vertices of different color in it.
  Timofey wants to find a vertex which he should take in hands so that
  there are no subtrees that annoy him. He doesn't consider the whole
  tree as a subtree since he can't see the color of the root vertex.

  A subtree of some vertex is a subgraph containing that vertex and all
  its descendants.

  Your task is to determine if there is a vertex, taking which in hands
  Timofey wouldn't be annoyed.

  Input
  The first line contains single integer n (2 ≤ n ≤ 10⁵) — the
  number of vertices in the tree.

  Each of the next n - 1 lines contains two integers u and v
  (1 ≤ u, v ≤ n, u ≠ v), denoting there is an edge between vertices u
  and v. It is guaranteed that the given graph is a tree.

  The next line contains n integers c1, c2, ..., cn (1 ≤ ci ≤ 10⁵),
  denoting the colors of the vertices.

  Output
  Print "NO" in a single line, if Timofey can't take the tree in
  such a way that it doesn't annoy him.

  Otherwise print "YES" in the first line. In the second line print the
  index of the vertex which Timofey should take in hands. If there are
  multiple answers, print any of them.

I have come up with a solution that works but times out on large inputs.

from collections import defaultdict

n = int(input())
graph = defaultdict(list)
colors = defaultdict(lambda: -1)
for i in range(n-1):
    (u, v) = [int(i) for i in input().split()]
    graph[u].append(v)
    graph[v].append(u)

c = [int(i) for i in input().strip().split()]
colors = {i+1: v for (i, v) in enumerate(c)}
bad = set()

def start(start):
    nebs = graph[start]
    return all(dfs(n, set([start]), colors[n]) for n in nebs)

def dfs(start, visited, want):
    visited.add(start)
    if colors[start] != want:
        return False
    for neb in graph[start]:
        if neb not in visited:
            if not dfs(neb, visited, want):
                return False
    return True

found = False
ans = -1

for i in range(1, n+1):
    if start(i):
        found = True
        ans = i
        break
    else:
        bad.add(i)

if not found:
    print("NO")
else:
    print("YES")
    print(ans)

I'd love some guidance on how I can make this faster -- thank you!

Some comments on the code:

for i in range(n-1):
    (u, v) = [int(i) for i in input().split()]

You are using i as the outer loop index, and i as the inner (list comprehension) loop index.  This is confusing, and should be discouraged.

The second line may be written more simply as:

    u, v = map(int, input().split())

The assignment:

colors = defaultdict(lambda: -1)

is overwritten by the code that creates the colors dictionary (below), and so can be removed.

You are changing the variable meaning in the following lines:

c = [int(i) for i in input().strip().split()]
colors = {i+1: v for (i, v) in enumerate(c)}

In the first line, i is a colour value; in the second line, v is the colour value and i is the index of the colour value.  For consistency, I'd used v instead of i in the first line.  Or get rid of the list comprehension index altogether:

c = map(int, input().strip().split())
colors = { i: v for i, v in enumerate(c, 1) }

Using enumerate(c, 1) eliminates the need to use i+1 as the key expression.

Is the .strip() necessary?

I'm not certain what the point of maintaining your visited set is.  Each time you start() a search from a new point, you are resetting the the visited set, and during any dfs(), you won't encounter a node you've already visited because the graph is guaranteed to be be a tree.  You just need to pass the start node for each dfs() step as a previous node (instead of a visited set) to prevent back-tracking.

Use better variable names.   I just figured out the nebs is an abbreviation for neighbours.

Faster solution hint:

I don’t believe a DFS is necessary.

Ignore all edges between vertices of the same colour
All other edges must share a common vertex (or Timofey will be annoyed)
Examine first two edges between different coloured vertices.  Continue with a linear search of remaining edges


Grouping a dictionary of lists based on  % key and the first value in the list How to best optimize this code ?

Problem Description:

Input(counts) is an array of \$n\$ integers, where each counts[i], (\$0\le i< n\$) denotes the total number of elements in a particular group that element i belongs to. For example, if counts = [3, 3, 3, 3, 3, 1, 3], then there are three groups; elements 0, 1, 2, 3, 4, and 6 are in one of two 3-element groups, and element 5 is in a 1-element group.

A group is valid if all the elements in the group have minimal ID numbers. In other words, a group of size \$k\$ must contain the \$k\$ smallest ID numbers belonging to a group of that size with respect to the smallest element ID in the group. For example, if counts = [3, 3, 3, 3, 3, 1, 3], then the grouping [0, 1, 2], [3, 4, 6], and [5] is valid; however, the grouping [0, 1, 4], [2, 3, 6], and [5] is not valid because the group [0, 1, 4] does not contain the three smallest element IDs for the set of element IDs belonging to 3-element groups (i.e., \${0,1,2,3,4,6}\$).

It is guaranteed that a valid grouping always exists for the given input array.

VALID GROUPING:

0 1 2
3 4 6
5

INVALID GROUPING:

0 1 4
2 3 6
5

Sample Input (1):

counts = [2,1,1,2,1]

Sample Output (1):

0 3
1
2
4

Sample Input (2):

counts = [4,2,4,5,5,4,4,5,5,2,5,7,1,7,1,7,1,7,1,7,1,7,1,7,5,5,5,5,5]

Sample Output (2):

0 2 5 6
1 9
3 4 7 8 10
11 13 15 17 19 21 23
12
14
16
18
20
22
24 25 26 27 28

Code:

from collections import OrderedDict

def groupCount(counts):
    group_dict = OrderedDict()
    for i in range(len(counts)):
        if counts[i] in group_dict.keys():
            group_dict[counts[i]].append(i)
        else:
            group_dict[counts[i]] = [i]
    op_list = []
    for key in group_dict:
        prnt_count = 0
        temp_list = []
        while len(group_dict[key])!=0:
            prnt_count += 1
            temp_list.append(group_dict[key].pop(0))
            if prnt_count % key == 0:
                op_list.append(temp_list)
                temp_list = []
    op_list.sort(key=lambda x: x[0])
    for value in op_list:
        print(*value, sep=' ')

1. Review

There's no docstring. What does groupCount do? The text in the post explains it very clearly, so using this would be a good start.
groupCount has two responsibilities: (i) it collates the input into groups, and (ii) it prints the collated results. The trouble with combining responsibilities like this is that it makes it hard to reuse the code. If you need to use the groups in other code, how do you get them? If you want to write automatic test cases, how do you do that? It would be better to split the grouping and printing code into separate functions. (This is the "single responsibility principle".)
This code iterates over the indexes in the sequence counts and then looks up the item using counts[i].

for i in range(len(counts)):
    # code using i and counts[i]

Python provides the function enumerate for simultaneously iterating over items and their indexes:

for i, count in enumerate(counts):
    # code using i and count

For collating items based on their keys, it is handy to use collections.defaultdict, like this:

groups = defaultdict(list)
for i, count in enumerate(counts):
    groups[count].append(i)

To split a sequence into groups of length \$n\$, there's a well-known trick that's described in the documentation for zip, where it says:

  This makes possible an idiom for clustering a data series into n-length groups using zip(*[iter(s)]*n). This repeats the same iterator n times so that each output tuple has the result of n calls to the iterator. This has the effect of dividing the input into n-length chunks.

There is no need to pass a key function to sort. Sequences are compared lexicographically by their items, and because we know that all the items are different, the comparison will never have to look past the first item.

2. Revised code

from collections import defaultdict

def group_by_count(counts):
    """Given a sequence of group sizes for each item, return a list of
    groups, each group being a tuple of indexes of items.

    >>> group_by_count([3, 3, 3, 3, 3, 1, 3])
    [(0, 1, 2), (3, 4, 6), (5,)]

    """
    groups = defaultdict(list)
    for i, count in enumerate(counts):
        groups[count].append(i)
    result = []
    for count, group in groups.items():
        result.extend(zip(*[iter(group)] * count))
    return sorted(result)

(Note the example in the docstring: this can be automatically checked using the doctest module.)


Haskell Graphing Module (Gloss) I've been slowly learning Haskell over the past few months. I made a small module to graph functions using Gloss, and I would like feedback on how idiomatic it is (looking for ways to simplify using standard functions and to make it more pointfree).

Graph.hs

module Graph (graph) where

import Graphics.Gloss

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) dx = pictures $ map Line visible
    where
        points :: [(Float, Float)]
        points = [(x, f x) | x <- [l,l+dx..r]]

        pairs :: [[(Float, Float)]]
        pairs = zipWith (\x y -> [x,y]) points $ tail points

        visible :: [[(Float, Float)]]
        visible = filter (all (\(_,y) -> b <= y && t >= y)) $ pairs

Main.hs

import Graphics.Gloss

import Graph

main :: IO ()
main = display FullScreen white . scale 20 20 . pictures $ [
        color blue $ graph f (-10, 10, -10, 10) 0.001,
        color black $ Line [(0, -10), (0, 10)],
        color black $ Line [(-10, 0), (10, 0)]
    ]
    where
        f :: Float -> Float
        f x = 1 / (x - 1)

Output

This looks great. Honestly, there isn't anything I'd change. You could make it more pointfree in some parts, but that's not going be more readable or maintainable.

Magic numbers

The only part I'd change are the static dimensions in main. Those are fine in a toy program, but a proper one will need some kind of configuration, so make sure that you use proper values from the beginning:

main :: IO ()
main = display FullScreen white . scale width height . pictures $ [
        color blue $ graph f (l, r, b, t) 0.001,
        color black $ Line [(origin, b), (origin, t)],
        color black $ Line [(l, origin), (r, origin)]
    ]
    where
        f :: Float -> Float
        f x = 1 / (x - 1)

        -- easy configurable:
        (l, r, b, t) = (-10, 10, -10, 10)
        width  = 20
        height = 20
        origin =  0

That way you can also exchange the values with proper command line interpretation

main :: IO ()
main = do
    (l, r, b, t) <- getDimensions
    let width = r - l
    let height = t - b
    let origin = ...
    display FullScreen white . scale width height . pictures $ [
...

After all, no magic numbers is a good practice in both imperative and functional languages.

Next, I'd introduce GraphBound as a type synonym, just to make graph's type signature easier on the eye:

-- | Graph boundaries, given in (left, right, bottom, top) order
type GraphBound = (Float, Float, Float, Float)

graph :: (Float -> Float) -> GraphBound -> Float -> Picture
graph f (l, r, b, t) dx = pictures $ map Line visible
    ...

You might even exchange GraphBound with a proper data type later which checks does not export its constructor to make sure that you don't end up with left = 20 and right = -10:

makeGraph :: Float -> Float -> Float -> Float -> Maybe GraphBound

However, that's an overkill, so let's not focus on that for too long.

List comprehensions vs. point-free

Now let's get back to your original query. Is it possible to make graph more point-free?

Sure:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) = pictures . map Line
                     . filter (all (\(_,y) -> b <= y && t >= y))
                     . (tail >>= flip (zipWith (\x y -> [x, y])))
                     . map (\x -> (x, f x))
                     . flip (enumFromThenTo l) r . (l+)

The point dx is completely gone from graph. However, the function is now unreadable. We went from a perfectly understandable function to a highly complex one. It gets a lot more readable if we use some helpers, but at that point we're almost back to your original function:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) = pictures . map Line . filter inGraph
                     . segments . points . ranged
   where
     inGraph (_,y) = fall (\(_,y) -> b <= y && t >= y)
     segments ps   = zipWith (\x y -> [x, y]) ps $ tail ps
     ...

That's not better than your original version, because your original version is already very good to begin with. The only change I could envision is a list comprehension in visible, but that's a matter of preference:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) dx = pictures lines
    where
        points = [(x, f x) | x <- [l,l+dx..r]]
        pairs  = zipWith (\x y -> [x,y]) points $ tail points

        lines = [Line segment | segment <- pairs,  all (\(_,y) -> b <= y && t >= y)) segment]

But you're the judge on which variant you want to use.

Other remarks

Thank you for using type signatures. Keep in mind that it's uncommon to use them in local bindings (where), as the outer function's signature should fix all types already. Inner type signatures can be a hassle if you change your outer type signature later, but they're sometimes necessary.


Counting from 0 to a number and back to zero, and space-padding the result to be a given length I completed a task: The objective was to create a method with 2 arguments: a number to count to, and how many char/numbers in the resulting string.

Result: Let's say that "number" is 5, so it will count: 01234543210 BUT it will also add spaces to the sides to complete the 2nd requirement "lenght", so if long is 15, the result would be " 01234543210 " with 2 spaces to each side (always same quantity of space each side) So i wrote this piece of code, but other coder told me that he used half the lines I did.

There were many assumptions in the code, like the fact that lenght is > than number or that if number is an odd number, lenght is odd too.

So my question would be, can this code be reduced (refactored?) (I just have one month of experience in Java, so be hard but understanding.)

public static void counter(int number, int lenght) {
    int numberFull=(number*2)+1;
    int spaceFull=lenght-numberFull;
    int space=spaceFull/2;
    for (int a=1;a<=space;++a)
        {System.out.print(" ");}
    for (int x=0;x<=number;x++)
        {System.out.print(x);}
    for (int y=number-1;y>=0;y--)
        {System.out.print(y);}
    for (int b=1;b<=space;++b)
        {System.out.print(" ");}
}

You can simplify the logic that prints the numbers going from 0 to number anc back to 0.

They are: number - Math.abs(number - x). Let's explain why. With the number 5, the wanted output is:

01234543210

As a pre-requisite, let's consider the (simpler) output

54321012345

We want to model that with a single variable x going from 0 to 2 * number. Clearly, there are 2 paths: one decreasing and one increasing.

                     5   4   3   2   1   0   1   2   3   4   5
value of x           0   1   2   3   4   5   6   7   8   9   10
value of 5 - x       5   4   3   2   1   0  -1  -2  -3  -4  -5
value of abs(5 - x)  5   4   3   2   1   0   1   2   3   4   5  <-- output

Now that we have that, it is clear that our real wanted output is simply 5 minus that one, i.e. number - Math.abs(number - x).

Putting this into code:

public static void counter(int number, int length) {
    int space = (length - number * 2 + 1) / 2;
    for (int a = 0; a < space; a++) {
        System.out.print(" ");
    }
    for (int x = 0; x <= number * 2; x++) {
        System.out.print(number - Math.abs(number - x));
    }
    for (int b = 0; b < space; b++) {
        System.out.print(" ");
    }
}

Other comments:

Add spaces before each operator, it adds to readability.
Don't use a mixed form of curly braces and single line statement. Either add curly braces everywhere and then have a proper line break or make it a single line without them.
There is no need to store into local variables when you only use it once. In this case having numberFull and spaceFull don't really add to clarity and it is much simpler to have int space = (length - number * 2 + 1) / 2;.
There is no validation on the number and the count of spaces. You might want to add some.
Instead of letting the counter method print the String directly, it would be better to let it build the String and return it. The caller can then decide to print it or not. For that, take a look at the StringBuilder class.


Find acronyms in Haskell Related to this code golf challenge, I tried to find acronyms with Haskell without using regular expressions.

The idea is to split the input string at every space or dash before finally gluing the heads of these parts together, if they are uppercase.

This is my code:

import System.Environment
import Data.Char

main :: IO ()
main = do
    [inp] <- getArgs -- get input from the command line
    putStrLn $ getAcronym inp

getAcronym :: String -> String
getAcronym [] = []
getAcronym s  = foldr step [] parts
    where parts = split isWordSep s -- split into words
          step x acc = if isUpper . head $ x
                       then head x : acc else acc -- glue uppercase heads together

split :: (a -> Bool) -> [a] -> [[a]]
split p []       = []
split p s@(x:xs)
  | p x       = split p xs -- discard trailing white spaces
  | otherwise = w : split p r -- continue with the rest
      where (w, r) = break p s -- seperate prefix

isWordSep :: Char -> Bool
isWordSep x = x == ' ' || x == '-'

As this really seems like a very simple problem, my code looks like way too much complexity.

Do you have any helpful improvements to slim down my code?

Since you asked for help slimming down your code, I'll give you some hints that might otherwise be inadvisable.

Don't use do-notation. In fact, you can replace

main :: IO ()
main = do
    [inp] <- getArgs -- get input from the command line
    putStrLn $ getAcronym inp

with

main :: IO ()
main = putStrLn =<< (fmap (getAcronym . head) getArgs)

I'd replace isWordSep to take advantage of pattern matching. As an example:

isWordSep :: Char -> Bool
isWordSep ' ' = True
isWordSep '-' = True
isWordSep _   = False

Consider using a parser library. Monadic parser combinators are quite nice, and they accomplish what you'd probably use regular expression for, but with far nicer error messages and type safety. Here's one example using megaparsec:

import           Control.Monad
import           Data.Void
import           Text.Megaparsec
import           Text.Megaparsec.Char

type Parser = Parsec Void String

uppercaseWord :: Parser Char
uppercaseWord = do
    h <- upperChar
    many $ noneOf "- "
    pure h

acronymP :: Parser String
acronymP = some $ do
    w <- uppercaseWord
    void (oneOf " -") <|> eof
    pure w

acronym :: String -> IO ()
acronym = parseTest acronymP

You can unpack the do-notation to get:

acronym :: String -> IO ()
acronym = parseTest (acronymP :: Parser String)
    where
        acronymP = some (uppercaseWord >>= ((void (oneOf " -") <|> eof) >>) . pure)
        uppercaseWord = upperChar >>= ((many (noneOf "- ") >>) . pure)

which, surprisingly enough, does indeed work.


Improve testability of constructor I have the following constructor

 /**
 * @param codePath path to player's code. If ends in '/' it assumes the code is in .class'es in
 *            a directory; otherwise assumes a jar
 * @throws PlayerException if there is a problem loading the code
 */
public Player(File codePath) throws PlayerException {
if (!codePath.exists() || !codePath.canRead()) {
    throw new IllegalArgumentException("Path is invalid: " + codePath
        + ". It can't be read or it doesn't exist");
}

if (Thread.currentThread().getPriority() < ROBOTS_MAX_PRIORITY) {
    throw new PlayerException("Robot priority cannot be greater than game's");
}

try {
    loader = new URLClassLoader(new URL[] { codePath.toURI().toURL() });
    InputStream stream = loader.getResourceAsStream(PLAYER_PROPERTIES_FILE);

    if (stream == null) {
    throw new PlayerException("Player Specification File not found: " + codePath + "!"
        + PLAYER_PROPERTIES_FILE);

    } else {
    Properties playerInfo = new Properties();
    playerInfo.load(stream);

    // read parameters
    author = playerInfo.getProperty("Author", "Unknown programmer");

    teamName = playerInfo.getProperty("Team", "Unknown team");

    robotsThreads = new ThreadGroup(PLAYERS_GROUP, teamName + " Threads");
    robotsThreads.setMaxPriority(ROBOTS_MAX_PRIORITY);

    String banksList = playerInfo.getProperty("Banks");

    if (banksList == null) {
        throw new PlayerException("Error loading configuration property: No banks definition found");
    }
    String[] banksClasses = banksList.split(",");

    teamId = getNextTeamId();
    teamIds.add(teamId);

    banks = loadBanks(loader, banksClasses);

    log.info("[Player] Successfully loaded Player: {}. Banks found = {}", this, banksClasses.length);
    }

} catch (ClassCastException | IOException | ClassNotFoundException | InstantiationException
    | IllegalAccessException | IllegalArgumentException | InvocationTargetException | SecurityException exc) {
    closeClassLoader();
    throw new PlayerException("Error loading player's code", exc);

} catch (PlayerException exc) {
    closeClassLoader();
    throw exc;
}

}

The problem is that the fact that it takes a File makes it hard to test, i don't want to create dummy files. My usual technique is to find what the logic does with the File and then create a constructor that takes that actual object and use that constructor in the unit tests with the original constructor calling the new one. In this case it can be seen that what's needed from the File is the loader so I tried creating a package-protected constructor

Player(URLClassLoader pLoader, String codePath) throws PlayerException { .. } //the second parameter is just used for logging

However, the problem is that I obviously can't do something like this

public Player(File codePath) throws PlayerException, MalformedURLException {
  if (!codePath.exists() || !codePath.canRead()) {
    throw new IllegalArgumentException("Path is invalid: " + codePath
        + ". It can't be read or it doesn't exist");
    }
this(new URLClassLoader(new URL[] { codePath.toURI().toURL() }), "test");

}

i.e. first validate the File and then invoke the other constructor, because this() calls need to be the first in the method.
I also tried extracting the initialization code into a separate method that then maybe i could test by itself, but the variables to initialize (author, teamName, etc) are final, so the compiler again complains. I'm trying to leave those variables as final.

Can anyone suggest another way to refactor this so that i can test the logic that populates the player's data at least. For the full class see here

I think you are putting way too much logic in your constructor. The other problem is that you do not want to create dummy files, if so, are there are real files you can make a copy of and test on? If there are no real files either you might actually have to create some real/dummy files.

And if I understand your question you want to test "the logic" of the Player class, which is basically invoking and creating a new URLClassLoader, but since this class takes a File as argument you can't invoke it. In that case you could change the argument from a File to a String so the invoke would look something like

public Player(String filePath) {
    this(new URLClassLoader(new URL[] { filePath }), "test");
}


Collecting YouTube links from a page I wrote code for my needs (i.e. collecting YouTube links from a page) and it is simple, but now I just want to know what can I do just to make it a better and properly written one. For example, I think I should have try/catch blocks/exceptions in my methods, or get/set properties. What tests can I write to test the code? I don't know to be honest, but how would the code look if you had written it?

public class Parser
{
    public static void Main()
    {
        Console.WriteLine("Enter the url of the page:");
        var url = Console.ReadLine();

        var numOfPages = GetNumberOfPages(url);
        var numOfLinks = 1;

        for (int page = 0; page <= numOfPages; page++)
        {
            if (page != 1)
            {
                if (page == 0)
                {
                    GetYouTubeLink(url).ForEach(x => Console.WriteLine($"{numOfLinks++}." +
                        $"{x.Replace("embed/", "watch?v=").Replace("?feature=oembed", "")}"));
                }
                else
                {
                    var newUrl = url + string.Format($"{page}/");
                    GetYouTubeLink(newUrl).ForEach(x => Console.WriteLine($"{numOfLinks++}." +
                        $"{x.Replace("embed/", "watch?v=").Replace("?feature=oembed", "")}"));
                    newUrl = string.Empty;
                }
            }
        }
    }

    private static HtmlDocument GetWebPageHtmlFromUrl(string url)
    {
        var webGet = new HtmlWeb();

        return webGet.Load(url);
    }

    private static int GetNumberOfPages(string url)
    {
        var pages = GetWebPageHtmlFromUrl(url)
            .DocumentNode.SelectNodes("//div[@class='page-link']")
            .Descendants("a")
            .Last().InnerText;

        return int.Parse(pages);
    }

    private static List<string> GetYouTubeLink(string url)
    {
        var links = new List<string>();
        var iframeLinks = GetWebPageHtmlFromUrl(url).DocumentNode.SelectNodes("//iframe");

        if (iframeLinks != null)
        {
            foreach (var link in iframeLinks)
            {
                links.Add(link.Attributes["src"].Value);
            }
        }

        return links;
    }
}

Comments inline:

public class Parser
{
    public static void Main()
    {
        Console.WriteLine("Enter the url of the page:");
        var url = Console.ReadLine();

        var numOfPages = GetNumberOfPages(url);
        var numOfLinks = 1;

        for (int page = 0; page <= numOfPages; page++)
        {
            // if (page != 1) is redundant in the original code, did you mean numOfPages?
            var newUrl = url + (page > 0 ? string.Format($"{page}/") : "");
            GetYouTubeLinks(newUrl).ForEach(x => Console.WriteLine($"{numOfLinks++}." +
                x.Replace("embed/", "watch?v=").Replace("?feature=oembed", "")));   // remove string interpolation since it's just one value
            // newUrl = string.Empty;       // Not needed
        }
    }

    private static HtmlDocument GetWebPageHtmlFromUrl(string url)
    {
        var webGet = new HtmlWeb();

        return webGet.Load(url);
    }

    private static int GetNumberOfPages(string url)
    {
        var pages = GetWebPageHtmlFromUrl(url)
            .DocumentNode.SelectNodes("//div[@class='page-link']")  // This parsing may work on specific pages, but it won't work on all pages. This is concerning since the user is prompted to enter any URL?
            .Descendants("a")
            .Last().InnerText;

        return int.Parse(pages);
    }

    private static List<string> GetYouTubeLinks(string url)     // Made plural
    {
        var iframeLinks = GetWebPageHtmlFromUrl(url).DocumentNode.SelectNodes("//iframe");

        if (iframeLinks == null)    // reduce nesting
        {
            return new List<string>(0);
        }

        return iframeLinks.Select(link => link.Attributes["src"].Value).ToList();   // simplify allocation
    }
}


Good way to structure a SDL project I am absolutely new to c++ and am trying to learn SDL2 with c++. I am setting up my first project and I was unsure if the way I have structured the project is good or if I have over done it. Here is the code:
generalFuncs.h
#pragma once

#include "SDL.h"
#include <iostream>

struct State
{
    bool quit = false;
};

class CreateWindow
{
public:
    CreateWindow(
        int x,
        int y,
        int w,
        int h,
        bool fullScreen = false
    )
    {
        pWindow = SDL_CreateWindow("", x, y, w, h, fullScreen);
    }

    inline SDL_Window* getWindow()
    {
        return pWindow;
    }
private:
    SDL_Window* pWindow;
};

class Events
{
public:
    void handleEvents(State* state)
    {
        while (SDL_PollEvent(&event))
        {
            if (event.type == SDL_QUIT)
                state->quit = true;
        }
    }

private:
    SDL_Event event;
};

renderer.h
#pragma once

#include "SDL.h"

class Renderer
{
public:
    Renderer(SDL_Window* pWindow)
    {
        pRenderer = SDL_CreateRenderer(pWindow, -1, 0);
    }

    inline SDL_Renderer* getRenderer()
    {
        return pRenderer;
    }

    inline void clearScreen(int r, int g, int b, int a)
    {
        SDL_SetRenderDrawColor(pRenderer, r, g, b, a);
        SDL_RenderClear(pRenderer);
    }

    inline void update()
    {
        SDL_RenderPresent(pRenderer);
    }

    void render()
    {

    }

private:
    SDL_Renderer* pRenderer;
};

main.cpp
#include <iostream>
#include <SDL.h>
#include <vector>
#include "generalFuncs.h"
#include "renderer.h"

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

    if (SDL_Init(SDL_INIT_EVERYTHING) != 0)
    {
        std::cout << "Could not initialze SDL properly \n" << SDL_GetError() << std::endl;
        return 0;
    }

    CreateWindow window(SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1200, 600);
    Renderer renderer(window.getWindow());
    State state;
    Events events;

    if (window.getWindow() == NULL)
    {
        std::cout << "Could not window properly \n" << SDL_GetError() << std::endl;
        return 0;
    }
    if (renderer.getRenderer() == NULL)
    {
        std::cout << "Could not window properly \n" << SDL_GetError() << std::endl;
        return 0;
    }

    while (!state.quit)
    {
        events.handleEvents(&state);
        renderer.clearScreen(255, 255, 255, 255);
        renderer.render();
        renderer.update();
    }

    SDL_DestroyWindow(window.getWindow());
    SDL_Quit();
    return EXIT_SUCCESS;

}

The way I plan on using this is make other header files, include them to render.h, then draw them in the render method. Please let me know if there are better way to do things or if my setup can be improved. Thank you.

I think that yes, the code can be structured somewhat better.
First of all, I'd tend to "RAII all the things". Code like this at the end of main makes me extremely suspicious:
    SDL_DestroyWindow(window.getWindow());
    SDL_Quit();

I'd strongly prefer that those be handled in the destructor of some object so they happen automatically.
So, I'd rename CreateWindow to just Window. Then I'd add a destructor that called SDL_DestroyWindow, so it'll automatically clean itself up when it goes out of scope.
class Window {
public:
    Window(int x, int y, int w, int h, bool fullScreen = false) {
        pWindow = SDL_CreateWindow("", x, y, w, h, fullScreen);
        if (pWindow == nullptr)
            throw std::runtime_error("Unable to create window");
    }

    ~Window() { SDL_DestroyWindow(pWindow); }

    operator SDL_Window*() { return pWindow; }

private:
    SDL_Window *pWindow;
};

Likewise I'd add an Application class (or something on that order that calls SDL_Init in its ctor, and SDL_Quit(); in its dtor, so simply defining a variable of that type automatically initializes SDL, and cleans up before it goes out of scope.
class Application {
public:
    Application() {
        if (SDL_Init(SDL_INIT_EVERYTHING) != 0)
        {
            std::stringstream buffer;
            buffer << "Could not intialize SDL properly: " << SDL_GetError();

            throw std::runtime_error(buffer.str());
        }
    }

    ~Application() {
        SDL_Quit();
    }
};

...and of course, Renderer should clean up after itself as well:
    ~Renderer() {
        SDL_DestroyRenderer(pRenderer);
    }

I'd also add definitions of copy assignment, move assignment, copy construction, and move construction to Renderer, Window and Application, so somebody won't accidentally do something nasty and breaking with them. In this case, they're probably move-only classes, so we'd do something on this order:
    Renderer(Renderer const &) = delete;
    Renderer(Renderer &&other) {
        pRenderer = other.pRenderer;
        other.pRenderer = nullptr;
    }
    Renderer &operator=(Renderer const &) = delete;
    Renderer &operator=(Renderer && other) {
        pRenderer = other.pRenderer;
        other.pRenderer = nullptr;
        return *this;
    }

Window would probably be similar in this regard, and Application should probably delete all of them.
I'd also have each of the classes report problems with construction by throwing an exception, rather than leaving it to client code to detect the problem when (for example) it has failed to create a window.
    Window(int x, int y, int w, int h, bool fullScreen = false) {
        pWindow = SDL_CreateWindow("", x, y, w, h, fullScreen);
        if (pWindow == nullptr)
            throw std::runtime_error("Unable to create window");
    }
}

Rather than Events::handleEvents being passed a Boolean in a structure, I'd have it just return a value to indicate when the program should exit:
    bool handleEvents()
    {
        while (SDL_PollEvent(&event))
        {
            if (event.type == SDL_QUIT)
                return false;
        }
        return true;
    }

I'd also put all of these in a namespace named something like sdl (or sdl2, or something on that order).
With that "stuff" in place, our main can shrink to something on this order:
int main(int argc, char* argv[])
{
    try {
        sdl::Application app;

        sdl::Window window(SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1600, 1200);
        sdl::Renderer renderer(window);
        sdl::Events events;

        while (events.handleEvents())
        {
            renderer.clearScreen(255, 255, 255, 255);
            renderer.render();
            renderer.update();
        }
    }
    catch(std::exception const &e) {
        std::cerr << e.what() << "\n";
    }
}

Although I haven't shown it above, I'd at least be tempted to require that a reference to an sdl::Application be passed to the ctor when you create an sdl::Window object. This assures that you've created an sdl::Application object first, thereby assuring that SDL is initialized before you try to create a window with it.


Hash Table or map implementation I'd like to get feedback on whether my hash table is correct or not. Also, any ideas on performance?

    template<class J,class K>
class List {
private:
    struct Node {
        J key;K data;

        Node * next;
        Node(J _key,K _data):key(_key),data(_data),next(nullptr) {}
    }*head;

public:
    List():head(nullptr){}
    ~List() {
        deleteNode();
    }
    void insert(J _key ,K _data) {
            Node * newNode = new Node(_key, _data);
            newNode->next = head;
            head = newNode;
    }

    void traverse() {
        for(Node * curr = head; curr != nullptr; curr = curr->next) {
            cout<< "( "<<curr->key << ", " << curr->data << " )" << " ";
        }
    }

    K search(J key) {
        if (!empty()){

            for(Node * curr = head;curr != nullptr;curr = curr->next) {
                if (curr->key == key) {
                    return curr->data;
                }
            }
        }

        return K(-1);
    }

    bool empty() { return (head == nullptr); }

    void deleteNode() {
        Node * tmp = nullptr;
        for(Node * curr = head; curr != nullptr;){
            tmp = curr->next;
            delete curr;
            curr = tmp;
        }
    }
};

template<class KEY, class VALUE>
class myHash {
private:

    struct bucket {
        List<KEY,VALUE> list_at_bucket;
    }*Buckets;

    int tableSize;
public:
    myHash(int size) {
        tableSize = size;
        Buckets = new bucket[tableSize];
    }

    ~myHash() {
        for(int i = 0; i < tableSize; i++) {
            Buckets[i].list_at_bucket.deleteNode();
        }
    }

    void put(KEY k, VALUE value) {
        int getCode = hash(k);
         Buckets[getCode].list_at_bucket.insert(k,value);
    }

    VALUE getValue(KEY _key) {
        int getCode = hash(_key);
        return  Buckets[getCode].list_at_bucket.search(_key);
    }

    bool contains(KEY _key) {
        int getCode = hash(_key);
        if (Buckets[getCode].list_at_bucket.search(_key) != -1) {
            return  true;
        }
        return false;
    }

    // need a template hash function
    int hash(KEY key) const {
        int value = 0;
        for (char each: key) {
            value += each;
        }
        return  value % tableSize;
    }

    void traverse() {
        for (int index = 0; index <tableSize; index++){
            cout<< "[" << index << " ]-> ";
            Buckets[index].list_at_bucket.traverse();
        }
    }
};

Well, there's a lott to say:

K and V are the traditional template-arguments for keys and values in C++.
Why do you use J and K respectively KEY and VALUE instead?
List::Node is only instantiated at one place, where all three members are set. Why then does it have a constructor at all, and one setting the last one to nullptr at that?
Default copy- and move- construction and assignment are completely unsuited to your List, as it owns its nodes. Leaving them leads to double-deletes and abandoned nodes. Look at the rule of three.

Also worth a look are inclass-initializers (for head).
deleteNode looks more like reset, and should zero head or be inlined into the destructor. As-is, it leaves head dangling...
The same comments apply to copying / moving / assigning myHash as to List.
The myHash dtor leaks the array of buckets...
contains should return the condition directly, instead of putting it into an if-clause. if(somebool) return true; else return false; is a bit wordy.
Your traverse-functions would be more appropriately named dump or some such... Anyway, why don't you overload operator>>(std::ostream, yourtypehere) instead?
Your hash-function runs afool of the fact that only the type itself can know which parts are relevant and how to hash them. Take a look at the design of std::hash.
Next, I would suggest embracing auto, see Almost Always Auto.


Regex Version of strip() This problem is from Automate The Boring Stuff using Python - Chapter 7.

  Write a function that takes a string and does the same thing as the strip()
  string method. If no other arguments are passed other than the string to
  strip, then whitespace characters will be removed from the beginning and
  end of the string. Otherwise, the characters specified in the second argument to the function will be removed from the string.

It takes a string and a character as input and return new stripped string.

#regexStrip.py - Regex Version of strip()

import re

def regex_strip(s, char=None):
    """
    Write a function that takes a string and does the same thing as the strip()
    string method. If no other arguments are passed other than the string to
    strip, then whitespace characters will be removed from the beginning and
    end of the string. Otherwise, the characters specified in the second argu-
    ment to the function will be removed from the string.
    """

    if not char:
        strip_left = re.compile(r'^\s*') #string starting with whitespace
        strip_right = re.compile(r'\s*$') #string ending with whitespace

        s = re.sub(strip_left, "", s) #replacing strip_left with "" in string s
        s = re.sub(strip_right, "", s) #replacing strip_right with "" in string s

    else:
        strip_char = re.compile(char)
        s = re.sub(strip_char, "", s)
    return s

if __name__ == '__main__':
    string_to_be_stripped = input("Enter string to be stripped: ")
    char_to_be_removed = input("Enter character to be removed, if none press enter: ")
    print(regex_strip(string_to_be_stripped, char_to_be_removed))

Output:

Enter string to be stripped: foo, bar, cat
Enter character to be removed, if none press enter: ,
foo bar cat

Pre-compilation

In theory, you're going to want to call this function more than once. That means that you only want to pay the cost of regex compilation once, and you should move your re.compile calls out of the function, setting your regex variables in the module's global scope.

Type-hinting

s is s: str, and char is (I think) also char: str.

'Removed from the string'?

I think this is the fault of unclear requirements, but - it would make more sense for the char argument to be the character[s] to strip from the string edges, not characters to remove from anywhere in the string. As such, you would need to re-evaluate how you create your regex.

Combine left and right

There's no need for two regexes. You can use one with a capturing group:

^\s*(.*?)\s*$


Method or helper function to return objects from EntityFramework with Linq The following code reads from Microsoft EntityFramework 6.0 domain model and returns the single view_patient object. The view_patient object is defined in the backend database and uploaded to the .edmx model of EF.

I am new to EF, but it seems there should be a better way--(better means less typing)-- of going from model to object without the intermediary anonymous data transfer object. As I have a couple hundred of these procedures to write (all within a WCF service), is there a better way of doing this? Or perhaps a "generic" method that will allow going directly from Link-to-entity to output of objects?

A note towards naming convention:
The classes are coming directly from Npgsql 3.0.5 EntityFramework interface -- which in turn is being read directly from PostgreSQL 9.5 database. I can not seem to get either EF or Npgsql to return PascalCase from the PostgreSQL database which is case-insensitive and, without adding quotes to all names, returns only lowercase names for tables, stored procedures, and columns.

Note #2: When not going through the intermediate DTO, I get the following error:

  The entity or complex type 'chaosModel.view_patient' cannot be
  constructed in a LINQ to Entities query.

(Using .NET 4.5)

  // testing the setup
    public view_patient Test(string cpatient)
    {
        using (var ctx = new chaosEntities())
        {

            var q = (from patient in ctx.patients
                     join chart in ctx.charts on patient.chart_recid equals chart.recid
                     where patient.cpatient == cpatient
                     select new
                     {
                         birthdate = patient.birthdate,
                         chart_number = chart.chart_number,
                         chart_recid = chart.recid,
                         city = patient.city,
                         cpatient = patient.cpatient,
                         donotsee = chart.donotsee,
                         firstname = patient.firstname,
                         groupid = chart.groupid,
                         lastname = patient.lastname,
                         mailbox = patient.mailbox,
                         mi = patient.mi,
                         patient_recid = patient.recid,
                         phone = patient.phone,
                         selfpay = chart.selfpay,
                         cashonly = chart.cashonly,
                         sex = patient.sex,
                         ssn = patient.ssn,
                         state = patient.state,
                         street = patient.street,
                         zipcode = patient.zipcode
                     });

            var r = (from c in q.AsEnumerable()
                     select new view_patient
                     {
                         birthdate = c.birthdate.ToShortDateString(),
                         chart_number = c.chart_number,
                         chart_recid = c.chart_recid,
                         city = c.city,
                         cpatient = c.cpatient,
                         donotsee = c.donotsee,
                         firstname = c.firstname,
                         groupid = c.groupid,
                         lastname = c.lastname,
                         mailbox = c.mailbox,
                         mi = c.mi,
                         patient_recid = c.patient_recid,
                         phone = c.phone,
                         selfpay = c.selfpay,
                         cashonly = c.cashonly,
                         sex = c.sex,
                         ssn = c.ssn,
                         state = c.state,
                         street = c.street,
                         zipcode = c.zipcode
                     }).SingleOrDefault();

            return r;
        }
    }

Besides the naming that should more C# friendly (PascalCase), the application should have a clear separation between data models and view models.

Data models are usually generated automatically in database first scenarios or POCOs classes defined by the programmer (code-first approach) and should be separated from view models or data returned by the service layer which usually consists in aggregated information.

Ok, for your particular case, data model is Patient:

public partial class Patient
{
    public DateTime BirthDate { get; set; }
    public String City { get; set; }
    public String Cpatient { get; set; }

    // rest of properties come here
}

and your view model can be called PatientViewModel:

public partial class PatientViewModel
{
    public DateTime BirthDate { get; set; }
    public DateTime BirthDateStr
    {
        get { return BirthDate .ToShortDateString(); }
    }

    public String ChartNumber { get; set; }
    public ChartRecid { get; set; }
    public String City { get; set; }
    public String Cpatient { get; set; }
}

So, as you can see, the view model hold the properties required by the view, which are aggregated from multiple data models.

Getting data model extra data

One method I found convenient (although not exactly orthodox, since it pollutes data models) is to extend the data model with properties that provide extra needed data, like this:

// this is another part of the partial class
public partial class Patient
{
    public String ChartNumber
    {
       get { return Chart.chart_number; }
       set { Chart.chart_number = value; }
    }

    public ChartRecid
    {
       get { return Chart.ChartRecid; }
       set { Chart.ChartRecid = value; }
    }

    // other "computed" properties come here
}

An alternative to this approach is to have custom map these properties when transferring data model -> view model and vice-versa (so, this also supports data persistence, not just data fetch).

Actual mapping

As already suggested, AutoMapper was specially designed for this kind of chores. Its usage is simple as:

setup the mapping:

(this is deprecated in the last version, as described here (and also mentioned by the compiler, if last NuGet package is referenced))

Mapper.CreateMap<Patient, PatientViewModel>();
Mapper.CreateMap<PatientViewModel>();

using it:

var patientDataModels = ctx.patients.
   .Include(p => p.Chart)               // eager load chart data
   .Where(p => cpatient == cpatient)
   .ToList();

var patientVms = new List<PatientViewModel>();
// Mapper should also handle collection mapping automatically, if type mapping is set up
Mapper.Map(patientDataModels, patientVms);

// if data model is not extended, you have to manually map Chart type properties

Eager load is recommended to avoid separate queries generated when Chart object properties are required (less SQL verbosity, significantly faster for large collections).

AutoMapper will automatically map properties having the same (and type, if type is different and no implicit conversion can be done, it will throw an exception) and not ignored (IgnoreMap attribute).


How to refactor a method with many prameters that sets a bit array I've written a method that is going to set a bit array by passing bool parameters to be able send correct command
private byte _mode;

     public void SetConfiguration(
            bool mode1,
            bool mode2,
            bool mode3,
            bool mode4,
            bool mode5,
            bool mode6,
            bool mode7,
            bool mode8)
        {
            var setBits = new BitArray(8);

            setBits.Set(0, mode1);
            setBits.Set(1, mode2);
            setBits.Set(2, mode3);
            setBits.Set(3, mode4);
            setBits.Set(4, mode5);
            setBits.Set(5, mode6);
            setBits.Set(6, mode7);
            setBits.Set(7, mode8);

            byte[] byteSet = new byte[1];
            setBits.CopyTo(byteSet, 0);
            _mode = byteSet[0];
        }

So depending on what command the application is sending I am going to set it with boolean. What bothers me is that the method has too many bool parameters and I'm wondering if anyone have an idea how to write this in a better way

You could use a params array to pass the parameters
public void SetConfiguration(params bool[] modes)
{
    var setBits = new BitArray(8);

    for (int i = 0; i < modes.Length; i++)
    {
        setBits.Set(i, modes[i]);
    }

    byte[] byteSet = new byte[1];
    setBits.CopyTo(byteSet, 0);
    _mode = byteSet[0];
}

You might want to add tests to ensure modes is not longer than 8 or you could expand the code to automatically set more than one byte in such cases. Shorter should be okay.
You can call it with individual parameters or with a bool array.
SetConfiguration(false, true, true, false, true, true, true, false);

var modes = new bool[] {false, true, true, false, true, true, true, false};
SetConfiguration(modes);


Updating items of one List that match another List I'm using VS 2008. I pass a List<T> a using ref, along with another List<T> b.

I need to cycle thru List<T> b, which contains around 200,000 items, and for each item in b, it it matches criteria in a, which contains 100,000 or more items, and update it.

public static void UpdateList(ref List<ExampleT> a, List<ExampleT> b)
{

    var count = 0;
    for (int i = 0; i < b.Count; i++)
    {

        foreach (var z in a.FindAll(x => x.MatchA == b[i].MatchA && x.MatchB == b[i].MatchB))
        {
            z.ToUpdate = b[i].ToUpdate;
            count++;
        }
        if(count >= 10000)
            break;
    }
}

Now, the only reason I added count was so that once 10000 items in a have been updated, the loop will stop, otherwise it will take a very long time to go thru all of the items. Even by limiting it like this, it still takes around 3 min.

For the sake of the example, T in this case is an object similar to the following:

public class ExampleT
{
public string MatchA {get;set;}
public string MatchB {get;set;}
public double? ToUpdate {get;set;} //in List a this will ALWAYS be null until it gets updated in the foreach
}

Is there a way I can make this more efficient?

Your code is O(n m), where n is the length of a and m is the length of b.

You could make it O(n + m) (assuming only a small number of items from a match each item in b) by using a hash table. One possible way to do this is to use ToLookup():

var aLookup = a.ToLookup(x => new { x.MatchA, x.MatchB });

foreach (var bItem in b)
{
    foreach (var aItem in aLookup[new { bItem.MatchA, bItem.MatchB }])
        aItem.ToUpdate = bItem.ToUpdate;
}

Another way to do basically the same thing would be using join:

var pairs = from bItem in b
            join aItem in a
              on new { bItem.MatchA, bItem.MatchB } equals new { aItem.MatchA, aItem.MatchB }
            select new { bItem, aItem };

foreach (var pair in pairs)
    pair.aItem.ToUpdate = pair.bItem.ToUpdate;

Also, you should really use better variable names.


Safely & portably read a value into a predefined integral type of varying size like mode_t I need to read a value from a text file and put it into a predefined type, in this particular case mode_t. This typedef can have a variety of types underlying it, so what is the portable way to read such a value and assign it to a variable of the predefined type.

Since you have no idea how large the type is, I was starting under the assumption that you should read using strtoimax/strtoumax and then range check somehow.  What I came up with "works" (seemingly) but looks ugly to my eye.

For example:

static mode_t parse_mode ( const char * s ) {
    char *       e = NULL;
    uintmax_t    u;
    uintmax_t    x;
    mode_t       m;
    int          c;

    if ( s == NULL) goto error_no_value;
    if (*s == '\0') goto error_no_value;
    if (*s == '-' ) goto error_negative;
    errno = 0;
    u = strtoumax(s, &e, 8);              /* value is in octal */
    if (errno) goto error_range;
    if (e == s) goto error_bogus;
    c = *e;
    if (c && !isspace(c)) goto error_bogus;   /* WS after is OK */

    /* now the question part -- better way??? */
         if (sizeof(mode_t) == 1) x = UINT8_MAX;
    else if (sizeof(mode_t) == 2) x = UINT16_MAX;
    else if (sizeof(mode_t) == 4) x = UINT32_MAX;
    else if (sizeof(mode_t) == 8) x = UINT64_MAX;
    else goto error_what_size;
    if (u > x) goto error_too_big;

    m = (mode_t)u;
    return m;
error_no_value: fprintf(stderr, "Error: no value\n");    return (mode_t)-1;
error_negative: fprintf(stderr, "Error: negative\n");    return (mode_t)-1;
error_range:    fprintf(stderr, "Error: range\n");       return (mode_t)-1;
error_bogus:    fprintf(stderr, "Error: bogus\n");       return (mode_t)-1;
error_what_size:fprintf(stderr, "Error: what size\n");   return (mode_t)-1;
error_too_big:  fprintf(stderr, "Error: too big\n");     return (mode_t)-1;
}

[I've totally glossed over the question of whether or not the type is signed or unsigned and just assumed unsigned here]

PS, yes, I have "secret knowledge" that no valid mode is likely to exceed 0177777; please ignore that as it is not necessarily true for other predefined types of its ilk.

PPS, junk like time_t which might not even be integral is not a part of this question

EDIT Added in the error goop.
Also, note that all leading whitespace has been skipped prior to this being called.

An alternative would be to store the value, then test whether any truncation occured:

m = u;
if (m != u) goto error_too_big;

I think some of the single-character variables (e, u, c, etc.) could be given better names.  The next person to modify the code (which could be Future You) will pick it up more quickly if it has meaningful variable names.


Simple Python module to test dictionaries I have some hardware for which I have a driver for which I have a library which can be used to easily configure and probe the device. Now I want to add support for this library to python because the management interface runs on a web server with a Python back-end, and I want to make the device manageable from the web interface, so it'll be easier if the back-end Python code can speak "directly" to the hardware.

Only issue is that I'm not certain about the ownership semantics in Python (2.7 if it matters, but I suspect that these semantics haven't changed much in 3.x), and this particular device needs to have very long uptime so I want to make sure I'm understanding the reference semantics correctly.

While the primary goal is to make sure I don't have any reference leaks, actually understanding the reference counting system is important to me. So if I'm unnecessarily releasing a reference, then that's interesting to know as well.

Specifically I'm wondering:

I assume that in get_conf() I don't need to incref the dict object when it's returned?  (I read somewhere that C functions act as regular scopes so new objects are deref'd when the function returns—is this accurate, and does this apply to returned objects?)
In set_conf(), do I need to decref the tmpstr? (Related to the point above). Let's say I create tmpstr once, and then reuse it and I don't decref it, will it be decref'd once the function returns?

I'm wondering if I have gotten references figured out correctly.

#include <stdbool.h>

#include <Python.h>

static PyObject *get_conf(PyObject *self, PyObject *args);
static PyObject *set_conf(PyObject *self, PyObject *args);

static PyMethodDef Methods[] = {
    {"get_conf", get_conf, METH_NOARGS,
     "Get configuration as a dictionary."},
    {"set_conf", set_conf, METH_VARARGS,
     "Set configuration from a dictionary."},
    {NULL, NULL, 0, NULL}        /* Sentinel */
};

bool g_enabled = true;
long g_val = 42;

PyMODINIT_FUNC initmyfoo(void)
{
  PyObject *m;

  m = Py_InitModule("myfoo", Methods);
  if(m == NULL) {
    return;
  }
}

static PyObject *get_conf(PyObject *self, PyObject *args)
{
  PyObject *dict;
  PyObject *b;

  dict = PyDict_New();
  if(dict == NULL) {
    /* should set exception before returning NULL! */
    return NULL;
  }

  if(g_enabled) {
    b = Py_True;
  } else {
    b = Py_False;
  }
  PyDict_SetItemString(dict, "enabled", b);
  PyDict_SetItemString(dict, "num_foo", PyLong_FromLong(42));

  return dict;
}

static PyObject *set_conf(PyObject *self, PyObject *args)
{
  PyObject *dict;
  PyObject *tmpstr;

  if(!PyArg_ParseTuple(args, "O", &dict)) {
    return NULL;
  }

  tmpstr = PyString_FromString("enabled");
  if(PyDict_Contains(dict, tmpstr) == 1) {
    PyObject* b;

    b = PyDict_GetItemString(dict, "enabled");
    if(PyObject_IsTrue(b)) {
      puts("enabling!");
      g_enabled = true;
    } else {
      puts("disabling!");
      g_enabled = false;
    }
  }
  Py_DECREF(tmpstr);

  tmpstr = PyString_FromString("num_foo");
  if(PyDict_Contains(dict, tmpstr) == 1) {
    PyObject* i;
    long val;

    i = PyDict_GetItemString(dict, "num_foo");
    if(PyInt_Check(i)) {
      long val;

      val = PyInt_AsLong(i);
      printf("setting num_foo=%u\n", (unsigned)val);

      g_val = val;
    } else {
      /* ToDo: Not a number exception */
      return NULL;
    }
  }
  Py_DECREF(tmpstr);

  Py_RETURN_NONE;
}

Just reviewing get_conf.

Support for Python 2.7 is planned to end in 2020, so it is not a good time to be starting new projects that run on Python 2.7 only.
The line:

PyDict_SetItemString(dict, "num_foo", PyLong_FromLong(42));

has three problems! First, PyLong_FromLong can fail (returning NULL), but your code does not check for this. Second, PyLong_FromLong returns a "new reference" which you never decrement, so this object will leak. Third, PyDict_SetItemString can also fail (returning -1), but your code does not check for this.

So, inconvenient as it may seem, you must write something like:

PyObject *num_foo = PyLong_FromLong(42);
if (num_foo == NULL) {
    /* handle failure somehow */
}
if (PyDict_SetItemString(dict, "num_foo", num_foo) != 0) {
    /* handle failure somehow */
}
Py_DECREF(num_foo);

One approach to writing a function that has many error cases is to have an error-handling section which is responsible for decrementing all the references that were acquired by the code. The error handlers should be arranged in reverse order, like this:

static PyObject *get_conf(PyObject *self, PyObject *args)
{
    PyObject *dict = PyDict_New(); /* (1) */
    if (dict == NULL) {
        goto fail_dict;
    }
    PyObject *enabled = g_enabled ? Py_True : Py_False;
    if (PyDict_SetItemString(dict, "enabled", enabled) != 0) {
        goto fail_set_enabled;
    }
    PyObject *num_foo = PyLong_FromLong(42);
    if (num_foo == NULL) {
        goto fail_num_foo;
    }
    if (PyDict_SetItemString(dict, "num_foo", num_foo) != 0) {
        goto fail_set_num_foo;
    }
    Py_DECREF(num_foo);
    return dict; /* (2) */

fail_set_num_foo:
    Py_DECREF(num_foo);
fail_num_foo:
fail_set_enabled:
    Py_DECREF(dict); /* (3) */
fail_dict:
    return NULL;
}

You can then check the reference-counting behaviour for each object. For example, the dict object gets one reference at (1), which is either returned to the caller at (2), or else decremented at (3). So this object does not leak.
The general inconvenience and delicacy of reference counting in C extension modules suggests that you might be better off with a hybrid approach where you implement a minimal hardware interface layer in C, and then package this up in Python to provide a friendlier interface.

The way this would work is that in C you would make a module _myfoo which contains functions like this:

static PyObject *get_enabled(PyObject *self, PyObject *args)
{
    return PyBool_FromLong(g_enabled);
}

static PyObject *get_num_foo(PyObject *self, PyObject *args)
{
    return PyLong_FromLong(g_val);
}

which are so simple that there is little risk of getting the reference counting wrong, and then in Python you would write a module myfoo containing:

import _myfoo

def get_conf():
    """Return hardware configuration dictionary with keys:

    enabled: bool -- [documentation here]
    num_foo: int -- [documentation here]
    """
    return dict(enabled=_myfoo.get_enabled(),
                num_foo=_myfoo.get_num_foo())

which you can see is much simpler than trying to allocate a dictionary in C.


Eliminate duplicates from strings I have written a piece of code for removing duplicated values from a string. Any suggestions on improvements?

public static String removeDuplicate(String s)
{
    char [] temp = s.toCharArray();
    int length =temp.length;
    for (int i=0;i<length;i++)
    {
        for (int j = i+1; j<length;j++)
        {
            if(temp[i]==temp[j])
            {
                int test =j;
                for(int k=j+1; k<length ; k++)
                {
                    temp[test] = temp[k];
                    test++;
                }
                length--;
                j--;
            }
        }
    }
    return String.copyValueOf(temp).substring(0,length);
}

Yusshi's code is perfectly fine. If you know that all the characters are in a specific range (e.g., ASCII code 0 to 255), you could also use either of the following codes which is faster. They uses an array rather than a hash set. The time complexity is O(n) and the space complexity is O(1). You cannot go lower in either case.

This one sorts the characters in the output string:

public static String removeDuplicates(String str) {
    int charsCount[] = new int[256];

    for (int i = 0; i < str.length(); i++) {
        char ch = str.charAt(i);
        charsCount[ch]++;
    }

    StringBuilder sb = new StringBuilder(charsCount.length);
    for (int i = 0; i < charsCount.length; i++) {
        if (charsCount[i] > 0) {
            sb.append((char)i);
        }
    }

    return sb.toString();
}

This one preserves the order of the characters:

public static String removeDuplicates(String str) {
    boolean seen[] = new boolean[256];
    StringBuilder sb = new StringBuilder(seen.length);

    for (int i = 0; i < str.length(); i++) {
        char ch = str.charAt(i);
        if (!seen[ch]) {
            seen[ch] = true;
            sb.append(ch);
        }
    }

    return sb.toString();
}


Creating custom random access iterator in C++ 2011 I would like an opinion on my code. I need a random access iterator for my custom container.
If I read the C++ 2011 standard, specially chapter 24, I understood that an implementation
could be the following:

class my_iterator : public std::iterator<std::random_access_iterator_tag, my_element>
{
    private:
       my_container *_ref;
       my_iterator_state state; // some useful state
    public:
       my_iterator ();
       my_iterator (const my_iterator &);
       my_iterator (my_iterator &&);
       ~my_iterator ();
       my_iterator &operator = (const my_iterator &);
       my_iterator &operator = (my_iterator &&);
       reference operator * ();
       my_iterator &operator ++ ();
       bool operator != (my_iterator);
       value_type operator * ();
       pointer operator -> ();
       my_iterator &operator * ();
       void operator ++ (int);
       value_type operator ++ (int);
       const my_iterator &operator ++ (int);
       reference operator ++ (int);
       my_iterator &operator -- ();
       const my_iterator operator -- (int);
       reference operator -- (int);
       my_iterator &operator += (difference_type);
       my_iterator operator + (difference_type);
       my_iterator operator - (difference_type);
       reference operator [] (difference_type);
       bool operator < (my_iterator);
       bool operator > (my_iterator);
       bool operator <= (my_iterator);
       bool operator >= (my_iterator);
       friend difference_type (my_iterator, my_iterator);
       friend my_iterator operator + (difference_type, my_iterator);
       friend void swap (iterator &, iterator &);
};

void swap (iterator &, iterator &);
difference_type (my_iterator, my_iterator);
my_iterator operator + (difference_type, my_iterator);

The above implementation is correct and full-featured? There are too members?

Sure but:

Sure:

   my_iterator ();
   bool operator != (my_iterator);
   pointer            operator -> ();
   my_iterator&       operator ++ ();
   my_iterator&       operator -- ();
   my_iterator&       operator += (difference_type);
   my_iterator        operator +  (difference_type);
   my_iterator        operator -  (difference_type);
   reference          operator [] (difference_type);
   bool               operator < (my_iterator);
   bool               operator > (my_iterator);
   bool               operator <= (my_iterator);
   bool               operator >= (my_iterator);

But most of the above can be defined in terms of each other and are basically forwarding functions to one definition.

The default versions of these look like they should work. So why are you defining them:

   my_iterator (const my_iterator &);
   my_iterator (my_iterator &&);
   my_iterator &operator = (const my_iterator &);
   my_iterator &operator = (my_iterator &&);

Do you really need a destructor?

   ~my_iterator ();

Three different version of operator* that only differ by return type!
That does not look valid.

   reference    operator * ();
   value_type   operator * ();
   my_iterator& operator * ();

Lots of operator++(int) that only differ by return type.
So again does not look valid

   void               operator ++ (int);
   reference          operator ++ (int);
   value_type         operator ++ (int);
   const my_iterator& operator ++ (int);
   reference          operator ++ (int);

Lots of operator--(int) that only differ by return type.
So again does not look valid

   const my_iterator operator -- (int);
   reference         operator -- (int);

Look here for a definition of what you need to support.


Calculating word-wise CRC This is a small program to calculate the CRC for an STM32 program image, compatible with the CRC on the STM32 hardware. It's a word-wise CRC, and the bin file is encoded as a series of 32 bit little endian words.

This is my first serious attempt at Rust and I'm looking for general comments, as well as a few questions:

Is there a better way of writing the repetitive (buff[3] as u32) when building the word?
Where should the T: Read trait bounds go? Should they also go on WordIterator?

use std::fs::File;
use std::io::Read;

struct WordIterator<T> {
    rd: T
}

impl<T: Read> WordIterator<T> {
    fn new(f: T) -> WordIterator<T> {
        WordIterator {rd: f}
    }
}

impl<T: Read> Iterator for WordIterator<T> {
    type Item = u32;

    fn next(&mut self) -> Option<u32> {
        let mut buf = [0 as u8; 4];
        match self.rd.read(&mut buf) {
            Ok(4) => {
                let v: u32 = (buf[3] as u32) << 24 |
                (buf[2] as u32) << 16 |
                (buf[1] as u32) << 8  |
                (buf[0] as u32) << 0;

                Some( v )
            },
            _ => {
                None
            }
        }
    }
}

struct CrcCalculator {
    lookup_table: [u32; 256]
}

impl CrcCalculator {
    fn new() -> CrcCalculator {
        let mut calc = CrcCalculator {lookup_table: [0; 256]};

        // Calculate the table:
        for v in 0..256 {
            let mut v2: u32 = v << 24;
            for _ in 0..8 {
                if 0 != v2 & 0x80000000 {
                    v2 = (v2 << 1) ^ 0x04C11DB7;
                }
                else {
                    v2 = v2 << 1;
                }
            }

            calc.lookup_table[v as usize] = v2;
        }

        calc
    }

    fn calculate<T: Read>(&self, f: T) -> (u32, u32) {
        let mut crc: u32 = 0xFFFFFFFF;
        let mut count = 0;

        for word in WordIterator::new(f) {
            crc = crc ^ word;
            crc = (crc << 8) ^ self.lookup_table[((crc >> 24) & 0xFF) as usize];
            crc = (crc << 8) ^ self.lookup_table[((crc >> 24) & 0xFF) as usize];
            crc = (crc << 8) ^ self.lookup_table[((crc >> 24) & 0xFF) as usize];
            crc = (crc << 8) ^ self.lookup_table[((crc >> 24) & 0xFF) as usize];

            count += 1;
        }

        (count, crc)
    }
}

fn main() {
    let lt = CrcCalculator::new();

    let f = File::open("Image.bin").unwrap();
    let (count, crc) = lt.calculate(f);

    println!("count=0x{:X} crc=0x{:08X}", count, crc);
}

Overall, this looks very reasonable! There are some small style nits like spaces inside of struct declarations. A few bigger points:

The Rust community loves crates and reusing existing work. In this case, you should use byteorder instead of bit-banging yourself. Beyond being less code, a nice bonus is that the fact that you use little-endian is now written in words in the code!
If you weren't doing this project for fun or learning, I'd recommend using one of the existing CRC crates to completely avoid reinventing the wheel!
Consider making the lookup table into a lazy_static global. This would allow it to only be initialized once.
Even better, consider hardcoding the lookup table. This would allow you to avoid any creation overhead. There are two paths:

You could copy-paste it in. Probably fine as the CRC table is unlikely to change often.
Create a build script that generates it at compile time and then include the result. This has a little bit of overhead at build time, but captures the process used to generate the table.

There's a few extra places that have type specifications when they aren't needed, such as when building the table. It's better to allow inference to apply when you can. Because you switch back and forth between usize and u32, there will still be some places it's needed.
You could extract a function for the repeated bit shuffling and/or use a loop. You may wish to profile if you are concerned about performance, as sometimes a manual loop unrolling is still required.
Use expect instead of unwrap, to help you and your users understand failures.
Add some buffering to your file reading to prevent inefficient amounts of system calls.
Try to name your generic types so that the name hints at the type. T is great when the type can be basically anything, but in this case I'd recommend R to reflect the Read bound.
Expand on your variable names a bit. rd has a larger scope as it's shared among multiple locations and should have an easier name.
I prefer to use the where clause to specify trait bounds. I find it easier to read.
I only add trait bounds where the function implementation requires it.

extern crate byteorder;

use byteorder::{LittleEndian, ReadBytesExt};

use std::fs::File;
use std::io::{Read, BufReader};

struct WordIterator<R> {
    inner: R,
}

impl<R> WordIterator<R> {
    fn new(r: R) -> WordIterator<R> {
        WordIterator { inner: r }
    }
}

impl<R> Iterator for WordIterator<R>
    where R: Read,
{
    type Item = u32;

    fn next(&mut self) -> Option<u32> {
        self.inner.read_u32::<LittleEndian>().ok()
    }
}

struct CrcCalculator {
    lookup_table: [u32; 256],
}

impl CrcCalculator {
    fn new() -> CrcCalculator {
        let mut calc = CrcCalculator { lookup_table: [0; 256] };

        // Calculate the table:
        for v in 0..256 {
            let mut v2 = v << 24;
            for _ in 0..8 {
                if 0 != v2 & 0x80000000 {
                    v2 = (v2 << 1) ^ 0x04C11DB7;
                } else {
                    v2 = v2 << 1;
                }
            }

            calc.lookup_table[v as usize] = v2;
        }

        calc
    }

    fn calculate<R>(&self, r: R) -> (u32, u32)
        where R: Read,
    {
        let mut crc = 0xFFFFFFFF;
        let mut count = 0;

        for word in WordIterator::new(r) {
            crc = crc ^ word;

            for _ in 0..4 {
                crc = (crc << 8) ^ self.next_component(crc);
            }

            count += 1;
        }

        (count, crc)
    }

    fn next_component(&self, crc: u32) -> u32 {
        self.lookup_table[((crc >> 24) & 0xFF) as usize]
    }
}

fn main() {
    let calculator = CrcCalculator::new();

    let f = BufReader::new(File::open("Image.bin").expect("Couldn't open input file"));
    let (count, crc) = calculator.calculate(f);

    println!("count=0x{:X} crc=0x{:08X}", count, crc);
}

For fun, I wanted to see what the compile-time build solution would look like.

build.rs

use std::env;
use std::io::{self, Write, BufWriter};
use std::fs::File;
use std::path::Path;

fn write_table<W>(mut w: W) -> io::Result<()>
    where W: Write,
{
    try!(writeln!(w, "static CRC_LOOKUP_TABLE: [u32; 256] = ["));

    for v in 0..256u32 {
        let mut v2 = v << 24;
        for _ in 0..8 {
            if 0 != v2 & 0x80000000 {
                v2 = (v2 << 1) ^ 0x04C11DB7;
            } else {
                v2 = v2 << 1;
            }
        }

        if v % 8 == 0 { try!(write!(w, "    ")) }
        try!(write!(w, "0x{:08X}, ", v2));
        if (v + 1) % 8 == 0 { try!(writeln!(w, "")) }
    }

    writeln!(w, "];")
}

fn main() {
    let out_dir = env::var("OUT_DIR").expect("Could not determine where to place output");
    let dest_path = Path::new(&out_dir).join("lookup_table.rs");
    let f = File::create(&dest_path).expect("Could not create lookup table file");
    let f = BufWriter::new(f);
    write_table(f).expect("Could not write lookup table")
}

And this means that the CRC functions don't need to be instance methods anymore, and can just be free functions:

include!(concat!(env!("OUT_DIR"), "/lookup_table.rs"));

fn crc_next_component(crc: u32) -> u32 {
    CRC_LOOKUP_TABLE[((crc >> 24) & 0xFF) as usize]
}

fn crc<R>(r: R) -> (u32, u32)
    where R: Read,
{
    let mut crc = 0xFFFFFFFF;
    let mut count = 0;

    for word in WordIterator::new(r) {
        crc = crc ^ word;

        for _ in 0..4 {
            crc = (crc << 8) ^ crc_next_component(crc);
        }

        count += 1;
    }

    (count, crc)
}


Correct logic in Monty Hall Problem? I've written this code in an attempt to create the Monty Hall problem and check the ratios. I would like someone to check the logic on it. I know it isn't organized well at all; I just need to see if I'm performing correctly.

If you don't know what the Monty Hall problem is.

private static int losses = 0;
private static int wins = 0;
public static void main(String[] args) throws Exception {
    for (int x = 0; x < 100; x++) {
        boolean doors[] = getDoors();
        run(getGuess(), doors);
    }
    System.out.println("Wins: " + wins);
    System.out.println("Losses: " + losses);
    double ratio = wins / wins + losses;
    System.out.println("Ratio: " + ratio);
}

public static boolean[] getDoors() {
    //door range
    int min = 1;
    int max = 3;
    //generate random correct door
    Random r = new Random();
    int i = r.nextInt(max - min) + min;
    //assign doors to boolean
    boolean[] doors = new boolean[3];
    for (int x = 0; x < 3; x++) {
        if (x == i) {
            doors[x] = true;
        } else {
            doors[x] = false;
        }
    }
    return doors;
}

public static int getGuess() {
    //guess range
    int min = 1;
    int max = 3;
    //generate random guess
    Random r = new Random();
    int guess = r.nextInt(max - min) + min;
    return guess;
}

public static void run(int guess, boolean[] doors) {
    //if guess is valid before switch, declare loss
    if (doors[guess]) {
        losses++;
        return;
    }
    //find other false door
    int wrongDoor = 0;
    for (int x = 0; x < 3; x++) {
        if (doors[x] == false && guess != x) {
            wrongDoor = x;
        }
    }
    //switch doors
    int newGuess = 0;
    for (int x = 0; x < 3; x++) {
        if (x != wrongDoor && x!= guess) {
            newGuess = x;
        }
    }
    //checkguess
    if (doors[newGuess] == true) {
        wins++;
    } else {
        losses++;
    }
}

You know it is not running correctly, the ratio is supposed to be 66%, not 50%.... what you really want is some help debugging it.....

Your issue is in your random number generation.... you are only ever setting, and choosing 2 of the three doors. Have a look at this common problem:

https://stackoverflow.com/questions/363681/generating-random-numbers-in-a-range-with-java

You are not using the ranges properly:

    //door range
    int min = 1;
    int max = 3;
    //generate random correct door
    Random r = new Random();
    int i = r.nextInt(max - min) + min;

in this code, max-min is 2, and nextInt(2) is going to be either 0, or 1., add that to min you get either 1 or 2.

You should not care about the min and max, you should just use:

int i = r.nextInt(doorcount); // assuming doorcount == 3

which will return 0, 1, or 2

If you fix this in both the getGuess and getDoors method then you will get the correct ratio of 66%.

Your code has a number of other problems too.

there is no need to create a new Random instance each time you want a random number. Create a single instance and share it.
do not call your method run. This will lead to confusion with the java.lang.Runnable interface
the line: double ratio = wins / wins + losses does not do what you think it does.

I think you should fix your program, then resubmit it for review....


GLFW simple event handler I wrote a simple concept of an event system in cpp.
It is uses a namespace and a global state, but normally a global state with namespaces is bad practice.

Is there a better way to do simple event handling with multiple callback functions?

header:

#ifndef EVENTS_EVENTMANAGER_H
#define EVENTS_EVENTMANAGER_H
#include <glad/glad.h>
#include <GLFW/glfw3.h>
#include <vector>
#include <functional>

namespace eventmanager
{

    //resize event
    static std::vector<std::function<void(GLFWwindow*, int, int)>> resizeEventCallbacks;
    void resizeEvent(GLFWwindow *window, int width, int hight);
    void subscribeResize(std::function<void(GLFWwindow*, int, int)> callback);
}

#endif //EVENTS_EVENTMANAGER_H

cpp:

#include "events/eventmanager.h"

void eventmanager::resizeEvent(GLFWwindow *window, int width, int hight)
{
    for (auto &callback: eventmanager::resizeEventCallbacks)
    {
        callback(window, width, hight);
    }
}

void eventmanager::subscribeResize(std::function<void(GLFWwindow *, int, int)> callback)
{
    eventmanager::resizeEventCallbacks.push_back(callback);
}

usage:

eventmanager::subscribeResize([&](GLFWwindow *window, int width, int hight) {
            ...

            });

I see one major issue:

static std::vector<std::function<void(GLFWwindow*, int, int)>> resizeEventCallbacks;

You should never put a static variable declaration like that in a header. That will cause every file that includes the header to have a local copy of that variable. What you probably want is to put that declaration in the .cpp file so that it is local to that file only. The resizeEventCallbacks is an implementation detail that should be known only to eventmanager.cpp functions.


Minimum number of steps to move from one city to another Question:
A country X consists of N cities numbered 0 to N-1 . The map of this country can be represented as a cycle — for each valid i, city i has exactly two neighboring cities (i+1)%N and (i-1+N)%N.

The cities in the country are broadly categorized into different types. For each valid i, the type of city i is denoted by A[i].

A person called Suarez wants to travel between some pairs of the cities. You are given Q queries. In each query, Suarez wants to travel from city number Y to a city of type Z. Also, he wants to travel only in a given direction along the cycle.

The direction is represented by a letter — L or R. If it's L, Suarez may only move from city i to city (i-1+N)%N for each valid i. If it's R, he may only move from city i to city (i+1)%N.

You need to find the minimum number of steps Suarez needs to take to reach a city of type Z if he starts moving from city Y in the given direction (he can take zero steps). In one step, Suarez can move from his current city to a neighboring city.

If Suarez can never reach a city of type Z for a given query, print -1 instead.

Input format

The first line of the input contains two space-separated integers N and Q. The next line contains N space-separated integers, where the i-th of these integers represents A[i].

Each of the following Q lines describes a query in the format Y Z D, where Y is an integer denoting the number of the starting city, Z is an integer denoting the type of a target city and D is a letter ('L' or 'R') denoting the direction along the cycle.

Output format

For each query, print a single line containing one integer — the answer to the query.

    Input                                Output
    3 4                                  0
    1 2 3                                2
    0 1 L                                1
    1 3 L                                -1
    2 1 R
    1 5 L

My solution is as follows. It gives TLE. Can you suggest me a better and optimized way of doing it.

    class TestClass {
public static void main(String args[] ) throws Exception {
    Scanner kb=new Scanner(System.in);
    int size=kb.nextInt();
    int queries=kb.nextInt();
    int[] city=new int[size];
    for(int i=0;i<size;i++){
        city[i]=kb.nextInt();
    }
    while(queries-->0){
        int current_city_number=kb.nextInt();
        int destination_city_type=kb.nextInt();
        char direction=kb.next().charAt(0);
        int number_of_steps=0;
        if(city[current_city_number]==destination_city_type){
            System.out.println("0");
        }
        else{
            boolean flag=false;
            for(int i=0;i<size;i++){
                if(destination_city_type==city[i]){  //Checking valid/invalid city type
                    flag=true;
                    break;
                }
            }
            if(flag){                                           //Valid city type
                    for(int i=current_city_number;i<size;){
                        if(direction=='R')
                        {i=(i+1)%size; number_of_steps++;}
                        else
                        {i=(i-1+size)%size; number_of_steps++;}
                        if(city[i]==destination_city_type){
                            System.out.println(number_of_steps);
                            break;
                        }
                    }
            }
            else{
                System.out.println("-1");                        //Invalid city type
            }
        }
    }
    }
    }

Okay, found problem online and solved it. The trick to the problem (as with most problems) lies in the constraints:

There are a lot of queries, but a small amount of cities. It would seem more important to optimize answering a query time, which is usually done with some precalculation or caching.

In this case, we can think that as there are at most 3000 cities, there is at most 3000 distinct city types. So all queries that don´t ask for an invalid city type (no city has that city type) will have one of those 3000 types.

We can precalculate for all cities, the query of going to right or left for each of these types, ending with a O(cities * distinct types * 2) complexity, which would be 3000 * 3000 * 2 at most, much better than the original 500000 * 3000.

Then for each query, if the type of city is a valid type (at least one of the cities has it), we have already preprocessed the answer and can answer in O(1). If it isn´t, we just answer with -1.

Another hint is the topic of the problem, which is for practicing multi dimentional arrays. We use this to store the calculation of all valid queries to then answer in O(1).

I advice you to try coding the problem again with these hints.

If you get stuck:

   Even though city types can go up to 200000, we can map them to values between 1 and 3000, because we only have 3000 cities.

And if you still can´t solve it, here is my solution:

        Scanner kb=new Scanner(System.in);
        int cities = kb.nextInt();
        int queries = kb.nextInt();

        int types[] = new int[cities];
        HashSet uniqueTypes = new HashSet();

        for (int i = 0; i < cities; i++) {
            types[i] = kb.nextInt();
            uniqueTypes.add(types[i]);
        }

        HashMap mapping = new HashMap();

        Integer idx = 0;
        for (Integer type: uniqueTypes) {
            mapping.put(type, idx++);
        }

        for (int i = 0; i < cities; i++) {
            types[i] = mapping.get(types[i]);
        }

        int closestRight[][] = new int[cities][cities];
        int closestLeft[][] = new int[cities][cities];

        for(int i = 0; i < cities; i++) {
            for (int j = 0; j < cities; j ++) {
                closestRight[i][j] = closestLeft[i][j] = cities + 1;
            }
        }

        for(int i = 0; i < cities; i++) {
            for (int j = 0; j < cities; j ++) {
                int leftIndex = ((i - j) + cities) % cities;
                int rightIndex = (i + j) % cities;
                int rightCityType = types[rightIndex];
                int leftCityType = types[leftIndex];

                closestRight[i][rightCityType] = Math.min(closestRight[i][rightCityType], j);
                closestLeft[i][leftCityType] = Math.min(closestLeft[i][leftCityType], j);

            }
        }

        BufferedWriter out = new BufferedWriter(new OutputStreamWriter(new
        FileOutputStream(java.io.FileDescriptor.out), "ASCII"), 512);

        for (int i = 0; i < queries; i++) {
            int city = kb.nextInt();

            int destinationType = kb.nextInt();

            if (mapping.get(destinationType) == null) {
                out.write("-1" + "\n");
                continue;
            }

            destinationType = mapping.get(destinationType);

            if (kb.next().charAt(0) == 'L') {
                out.write(closestLeft[city][destinationType] + "\n");
            } else {
                out.write(closestRight[city][destinationType] + "\n");
            }

        }

        out.flush();