Chaining method calls and "bubbling" the results I need a way to build up a sort of stack of places to search for an item.

What I would like to do is to have each layer searched for an item, and if it is found at that level, then it should be loaded by each higher level (successively).

What I have so far is just a basic framework, but I was wondering if I was missing some more apt abstraction here in terms of command flow. Does anyone have any suggestions?

type Result = String
type Query = String
type Search = Query -> Maybe Result
type Load = Result -> Maybe Result

data LoadFunc = LoadFunc {
  search :: Search,
  load :: Load }

data RunFunc = RunFunc { runner :: [RunFunc] -> Maybe Result }

mkRunFunc :: Query -> LoadFunc -> RunFunc
mkRunFunc q lf = RunFunc r
  where r (x:xs) =
          case search lf q of
            Nothing -> runner x xs >>= load lf
            a       -> a
        r []    = search lf q

inMem :: LoadFunc
inMem = LoadFunc { search = search, load = load }
  where search q = if q == "hi" then return $ "Found in mem." else Nothing
        load = addToLog "Loaded to mem."

renDisk :: LoadFunc
renDisk = LoadFunc { search = search, load = load }
  where search q = if q == "foo" then return $ "Found rendered." else Nothing
        load = addToLog "Added rendered."

onDisk :: LoadFunc
onDisk = LoadFunc { search = search, load = load }
  where search q = if q == "bar" then return $ "Found on disk." else Nothing
        load = addToLog "Now on disk."

addToLog :: Result -> Result -> Maybe Result
addToLog next str = return $ str ++ (' ':next)

tester :: [LoadFunc] -> Query -> Maybe Result
tester caches q = runner first rem
  where caches' = mk `fmap` caches
        first   = head caches'
        rem     = tail caches'
        mk      = mkRunFunc q

To see this in action, it should be sufficient to load up ghci and enter:

tester "bar" [inMem,renDisk,onDisk]

How about making inMem & co "bracket" functions? Like follows:

import Control.Monad.Writer

type M a = Writer String a
type LoadFunc a b = (a -> M b) -> a -> M b

type Query = String
type Result = ()

inMem :: LoadFunc Query Result
inMem _ "hi" = tell "Found in mem. "
inMem f q    = f q >> tell "Added to mem. "

renDisk :: LoadFunc Query Result
renDisk _ "foo" = tell "Found rendered. "
renDisk f q     = f q >> tell "Added rendered. "

onDisk :: LoadFunc Query Result
onDisk _ "bar" = tell "Found on disk. "
onDisk f q     = f q >> tell "Now on disk. "

tester :: [LoadFunc Query Result] -> Query -> M Result
tester = foldr ($) notFound
  where notFound q = tell "Not found!"

Run using runWriter $ tester [inMem,renDisk,onDisk] "bar"

This should give you the control flow you seek - as well as enough room for extension by replacing the monad as required. You could for example have notFound throw an exception using an Error monad instead (which would probably make sense).


Generate a deck of cards in Clojure / Clojurescript I would like to generate a deck of cards:

{:card-id 1 :suit :spade :rank 1}
{:card-id 2 :suit :spade :rank 2}
...
{:card-id 52 :suit :club :rank 13}

And here is my attempt:

(defn new-deck []
  (let [cards (flatten
                 (for [suit [:spade :heart :diamond :club]]
                    (for [rank [1 2 3 4 5 6 7 8 9 10 11 12 13]]
                      {:suit suit :rank rank} )))]
     (map-indexed (fn [idx itm] {:card-id (inc idx)
                                 :suit    (:suit itm)
                                 :rank    (:rank itm)})
                  cards)))

The code works but I think it is awkward. Especially the part where I use map-indexed function to add :card-id to each card.

Any suggestions to improve the code snippet?

There are a few minor improvements you can make here that lead to a fairly elegant implementation.

First, using flatten is rarely a good idea, because rather than just dealing with the top-level structure of the thing you hand it, it reaches down into its inner structure, thus making it brittle when you change the representation of your data. Therefore, you should instead use apply concat in place of flatten in your code.

However, as @ChrisMurphy points out, the for macro already supports list products, so you can replace the usage of flatten or apply concat and the two usages of for with a single usage of for that produces a flat sequence.

Then, instead of writing out the full range of numbers from 1 to 13, you can use the range function to replace that vector with (range 1 (inc 13)) or something equivalent.

Finally, rather than manually deconstructing and reconstructing each map in your lambda that you pass to map-indexed, you can use the assoc function to simply add the :card-id mapping.

With these changes, you end up with a solution that looks like this:

(defn new-deck []
  (map-indexed
   #(assoc %2 :card-id (inc %1))
   (for [suit [:spade :heart :diamond :club]
         rank (range 1 (inc 13))]
     {:suit suit :rank rank})))


Test to see if installed and if not, install it Okay all you Linux/shell folks out there. This is my first shell script (2nd if you count "Hello World") and would appreciate some feedback.

Basically, I need to test whether htop is installed; if it is, then move on; if it is not, then install it. If the user is root then just do it, and if not, sudo and install it. I have "borrowed" code from here to stitch this together. It works in my test environment. I would like to know:

What did I miss? What other variables do I need to account for? Are there better ways to go about this?

if ! [ -x "$(command -v htop)" ]; then
    # set the var SUDO to "" (blank)
    SUDO=""
        # if the user ID is not 0 (root is 0)
        if [ $EUID != "0" ] ; then
        # set the var SUDO to "sudo"
        SUDO="sudo"
        fi
    # Let the user know it is not installed & then install it, using sudo if not root
    echo 'Error: htop is not installed.' && $SUDO apt install htop >&1
    # exit the script
    exit 0
fi

Overall it's fine. My suggestions would be:

Fix the indenting.
If you find the comments helpful, great. Otherwise, consider saying "why" and not "what", e.g. # set the var SUDO to "" (blank) -> # This will be used as a prefix to run a command as root
You can alternatively consider checking if the command exists by running it: if htop --version > /dev/null 2>&1. This does not discriminate against aliases, functions and builtins.
Prefer user lowercase variable names to avoid conflicting with existing environment variables. SUDO="sudo" is ok since SUDO isn't in use, but if you had tried the same with PWD="pwd" or SHELL="sh" you could have gotten some odd side effects.
Use echo 'Error: htop is not installed.' >&2 to write the error message to stderr
The >&1 aka 1>&1 is redundant: it just redirects the FD to where it's already going.
It's generally considered rude to install packages without asking the user. A more canonical approach is to simply exit with an error saying that htop is missing and leave it to the user to install it (this also avoids tying the script to Ubuntu/Mint).
If you continue when htop exists, why is the script exiting after installing htop? If it's to handle installation failures due to bad password or full disk, you should probably handle that explicitly.
You can use exit aka exit $? to exit with the status of the previous command, so that if the installation failed, you don't claim that the script ran successfully.


2D Array: Retrieve the "Left" Item I am creating a game that contains a Board and Pieces. The Board is basically a 2D array of Pieces. [Think smaller chess board]

One of the things that I want to accomplish is to be able to retrieve the left piece of a given.. (Also want to be able to retrieve the right, up, and down).

The easiest way to achieve this is to simply do something like:

board[i-1][j] //which will give me the piece that's to the left of board[i][j]

The problem with just doing this is that this can get ugly and there is no error checking (array out of range).

I have a feeling that there may be a better data structure to use than a 2D array.

Also, I have two implementations that work, but neither really seem like a good idea

I have the following (untested) Java classes, which is a small scale of my application:

the class Board:

public class Board
{
protected Piece[][] board;

public Board()
{
    board = new Piece[7][7];
    int ctr = 0;
    for(int i = 0; i < 7; i++)
    {
        for(int j = 0; j < 7; j++)
        {
            board[i][j] = new Piece(i, j, ctr++);
        }
    }
}

public Piece PieceAt(int i, int j)
{
    return board[i][j];
}

public Piece LeftOf(Piece p)
{
    return PieceAt(p.verticalIndex, p.horizontalIndex - 1);
}
}

And the Piece class

public class Piece
{
    int verticalIndex;
    int horizontalIndex;
    int value;

    public Piece(int i, int j, int value)
    {
        this.horizontalIndex = i;
        this.verticalIndex = j;
        this.value = value;
    }

    public Piece GetLeftPiece(Board b)
    {
        return (this.horizontalIndex == 0 ? null : b.PieceAt(this.horizontalIndex, this.verticalIndex - 1));
    }
}

The first implementation sits in Board.java. The problem with this is that If I want to call it, it's a big ugly: b.LeftOf(b.PieceAt(3,3)) where b is of  type Board.

The second implementation seems like a better idea, however since the Pieces no nothing about each other, I have to include board in the method signature.

The first location of the leftOf method is the way to go. Putting it in Piece isn't very cohesive. The latter would be just as ugly or uglier (b.pieceAt(3,3).getLeftPiece(b)). The syntax board.leftOf(b.pieceAt(3,3)), read aloud, communicates roughly your intent but might read better as board.getPieceLeftOf(b.pieceAt(3,3)). Further, the best way is to remove the middle man and change the interface:

public Piece getPieceLeftOf(int x, int y) {
    int leftLocation = x - 1;
    // Validate that location
    return pieceAt(leftLocation, y);
}

//  ...later
board.getPieceLeftOf(3, 3);

Some style notes/thoughts:

Since this is written in Java, you should be following the standard naming conventions. camelCase for method names
You have a lot of package and package-private fields, it's a better practice to make them private and then expose the ones that actually need to be exposed outside of the class. (Effective Java Item 13).
If there is no error checking for the pieceAt or leftOf methods then add it and communicate through javadocs what happens if there is no left piece, a request is made out of bounds, etc. Those are both implemenation and game-type rules that need to be considered.
It seems like a simple 2D array is fine for this application. That is a pretty common structure for 2D chess/checkers type games (quick access, easy mental model, etc).


Generate all valid combinations of n pair of parentheses Implement an algorithm to print all valid (e.g. properly opened and
  closed) combinations of n pair of parentheses.

Following is my implementation and I think it works. Is the algorithm efficient? Do you see any coding issues?

def compute_all_parens (n, left, right, s):
    if right == n:
        print (s)
        return
    if left < n:
        compute_all_parens(n, left+1, right, s + "(")
    if right < left:
        compute_all_parens(n, left, right+1, s + ")")

# call syntax: compute_all_parens(5, 0, 0, "")

API

The compute_all_parens function requires 4 parameters.
From a user's perspective,
it would be more natural to require only one,
to avoid confusion about what values to pass for the others.
You could use an inner function to encapsulate your implementation details:

def print_all_parens(n):
    def print_parens(left, right, s):
        if right == n:
            print(s)
            return
        if left < n:
            print_parens(left + 1, right, s + "(")
        if right < left:
            print_parens(left, right + 1, s + ")")

    print_parens(0, 0, "")

I also corrected some formatting issues, following PEP8.

Use generators

Instead of printing, it would be nicer to return a generator:

def compute_all_parens(n):
    def compute_parens(left, right, s):
        if right == n:
            yield s
            return
        if left < n:
            yield from compute_parens(left + 1, right, s + "(")
        if right < left:
            yield from compute_parens(left, right + 1, s + ")")

    yield from compute_parens(0, 0, "")

This will give callers the freedom to do whatever they want with the combinations.


Python compare every line in file with itself I am implementing a statistical program and have created a performance bottleneck and was hoping that I could obtain some help from the community to possibly point me in the direction of optimization.

I am creating a set for each row in a file and finding the intersection of that set by comparing the set data of each row in the same file. I then use the size of that intersection to filter certain sets from the output. The problem is that I have a nested for loop(\$O(n^2)\$) and the standard size of the files incoming into the program are just over 20,000 lines long. I have timed the algorithm and for under 500 lines it runs in about 20 minutes but for the big files it takes about 8 hours to finish.

I have 16GB of RAM at disposal and a significantly quick 4-core Intel i7 processor. I have noticed no significant difference in memory use by copying the list1 and using a second list for comparison instead of opening the file again(maybe this is because I have an SSD?). I thought the with open mechanism reads/writes directly to the HDD which is slower but noticed no difference when using two lists. In fact, the program rarely uses more than 1GB of RAM during operation.

I am hoping that other people have used a certain datatype or maybe better understands multiprocessing in Python and that they might be able to help me speed things up. I appreciate any help and I hope my code isn't too poorly written.

import ast, sys, os, shutil
list1 = []
filterValue = 3
end = 0

# creates output file with filterValue appended to name
with open(arg2 + arg1 + "/filteredSets" + str(filterValue) , "w") as outfile:
    with open(arg2 + arg1 + "/file", "r") as infile:
        # create a list of sets of rows in file
        for row in infile:
            list1.append(set(ast.literal_eval(row)))

            infile.seek(0)
            for row in infile:
                # if file only has one row, no comparisons need to be made
                if not(len(list1) == 1):
                    # get the first set from the list and...
                    set1 = set(ast.literal_eval(row))
                    # ...find the intersection of every other set in the file
                    for i in range(0, len(list1)):
                        # don't compare the set with itself
                        if not(pos == i):
                            set2 = list1[i]
                            set3 = set1.intersection(set2)
                            # if the two sets have less than 3 items in common
                            if(len(set3) < filterValue):
                                # and you've reached the end of the file
                                if(i == len(list1) - 1):
                                    # append the row in outfile
                                    outfile.write(row)
                                    # increase position in infile
                                    pos += 1
                            else:
                                break
                    else:
                        outfile.write(row)

Sample input would be a file with this format:

[userID1, userID2, userID3]
[userID5, userID3, userID9]
[userID10, userID2, userID3, userID1]
[userID8, userID20, userID11, userID1]

The output file if this were the input file would be:

[userID5, userID3, userID9]
[userID8, userID20, userID11, userID1]

...because the two sets removed contained three or more of the same user IDs.

On reading the file twice

Constructing data structures is costly. You do it once for each line in the file (\$n\$ times) and then do it again \$n^2\$ times. There is absolutely no need as you could only use one list to do so:

>>> a = [[1], [2], [3]]
>>> for i in a:
...     for j in a:
...         print(i, j)
...
[1] [1]
[1] [2]
[1] [3]
[2] [1]
[2] [2]
[2] [3]
[3] [1]
[3] [2]
[3] [3]

That way, with only one list, you avoid spending time converting strings to datastructure and can focus on your algorithm.

So just build the list once and use it any time you want.

Use functions

It lets you reuse your work more easily, by changing parameters. You can also more easily add code to handle command-line arguments and not need to change your code each time you change your input/output file.

A simple layout could look like:

def parse_input(filename):
    ...

def compute_output(output_file, data, filter_value):
    ...

def filter_file(path, filter_value=3, in_name='file', out_name='filteredSets'):
    data = parse_input('{}/{}'.format(path, in_name))
    with open('{}/{}{}'.format(path, out_name, filter_value), 'w') as out_file:
        if len(data) > 1:
            compute_output(out_file, data, filter_value)
        else:
            for datum in data:
                print(datum, file=out_file)

if __name__ == '__main__':
    filter_file(arg2 + arg1)

I’m assuming Python 3 here for the print(datum, file=out_file) line. Change it accordingly if you are using Python 2. It, however, will change your output a bit, but more on that later. I'm also using a for loop in the else just in case there is no rows in data.

Separating concerns with functions let you focus on being efficient for simple tasks. And, if later you want to improve your code thanks to multiprocessing, it will be easier to process it in parallel (using map_async for instance).

Iterate directly over items

Rather than iterating over indices and using these indices to retrieve items. Instead of:

for i in range(0, len(list1)):
    ...
    set2 = list1[i]

You should use:

for set2 in list1:

It's both easier to read and faster. If you truly need the index, you can still use enumerate:

for i, set2 in enumerate(list1):

But as far as eliminating tests against the same line in our product you can test if the object is the same:

for set1 in list1:
    for set2 in list1:
        if set1 is not set2:
            # Do something

Note that I used is instead of == to be sure that we are performing tests on the same line and not on lines with identical values:

>>> a = [[1], [1], [1]]
>>> for i in a:
...     for j in a:
...         print(i, j, i==j, i is j)
...
[1] [1] True True
[1] [1] True False
[1] [1] True False
[1] [1] True False
[1] [1] True True
[1] [1] True False
[1] [1] True False
[1] [1] True False
[1] [1] True True

Managing "stop-early" in loops

Python allows loops (for and while) an optional else clause. This clause executes only if the loop executed entirely without reaching a break (or a return, for what it's worth).

In your case, you could use that at your advantage in the inner for loop: instead of waiting to reach the end of the file for each line, you can break as soon as you find that two lines shares more than 3 items. If you handle writing lines in the else clause of the inner for loop, you can rely on that break to not print a line that has too many items in common with an other one.

Miscellaneous

You can use os.path.join to concatenate folders and filenames.
You can use set1 & set2 as a shorthand to set1.intersection(set2).
You should write each import statement on its own line.

Proposed improvements

import os
from ast import literal_eval

def parse_input(filename):
    with open(filename) as f:
        data = [set(literal_eval(line)) for line in f]
    return data

def compute_output(output_file, data, filter_value):
    for set1 in data:
        for set2 in data:
            if set1 is set2:
                # Do not try to compare a row with itself
                continue
            if len(set1 & set2) >= filter_value:
                # Do not keep lines that have more than a few items
                # in common with any other line
                break
        else:
            # If the second for loop did not break, then keep the line
            print(set1, file=output_file)

def filter_file(path, filter_value=3, in_name='file', out_name='filteredSets'):
    data = parse_input(os.path.join(path, in_name))
    output_filename = os.path.join(path, '{}{}'.format(out_name, filter_value))
    with open(output_filename, 'w') as out_file:
        compute_output(out_file, data, filter_value)

if __name__ == '__main__':
    filter_file(arg2 + arg1)

You should have noticed that I changed the output format a bit. Now each line is a set instead of a list. It's easy to change that by calling print(list(set1), file=output_file) instead. It may change the order, but wont change the content. You could even use sorted(set1).

Beyond ast.literal_eval

I don't know if you can manage the input files of if you have to build around their limitations. But in case you can change them, I suggest you have a look at the pickle module.

Basicaly, when creating your input file, you can pickle.dump a list of all rows (possibly already a list of sets) and then, load them in this script using a simple data = pickle.load(os.path.join(arg2 + arg1, 'file')).


Top Python badged users from NYC I forked this StackExchange Data Explorer (SEDE) query from another one, and made it a bit more complex. I'm looking for feedback on my style, as well as answers to other questions (see below).

-- top users: NYC
-- forked from Avinash Raj's query for Chennai
select
    row_number() over(order by u.Reputation desc) as [#],
    u.Id as [User Link],
    u.Reputation,
    u.Location
from -- denormalize the Users with their Badges, i.e., flatten into a single table.
    Users u
  join
    Badges b
  on
    u.Id = b.UserId
where
    b.name = N'python'
  and
    b.class = 3 -- 1 is gold, 2: silver, 3: bronze
  and
    u.Reputation >= 1000
  and
    (
      lower(u.Location) like '%nyc%'
      or upper(Location) like '%NY, NY%'
      or lower(Location) like '%staten%'
      or lower(Location) like '%bronx%'
      or lower(Location) like '%queens%'
      or lower(Location) like '%new york, ny%'
      or lower(Location) like '%manhattan%'
      or lower(Location) like '%brooklyn%'
    )
order by
    u.Reputation desc;

Questions:

Would this sql work with all relational DB's that accept SQL?
If not, what is non-standard? How would I standardize it?
I don't like all-uppercase queries - I find them more difficult to read, so I used the lowercase, is that problematic?
Is it possible, in a standard way, to factor pieces out of this query? Or nonstandard way specific to SEDE (or other system)? For example, could I avoid doing lower(Location) repeatedly? What about aliasing my joined table outside the code block for usage inside of it? I have order by u.Reputation desc twice, for example, can I factor that out?

I have just got some feedback on using table identifiers so that people don't have to guess, so I'll try to do that from now on. Am I doing it anywhere that's silly?

I was asked about allowing user input for the badge name - Yes, I intended to do that too - as well as specify type of badge (gold, silver, bronze) but I ran out of time, and don't have a clear idea how to implement mapping gold/silver/bronze to 1/2/3. I also have false positives and was hoping someone could point me at a more concise and/or accurate way to filter.

Questions
First to address some of your questions:

Would this sql work with all relational DB's that accept SQL?

No, it would not. In fact it's quite unrealistic (if not impossible) to write SQL queries that would port over to multiple DBMS, as they all have different syntax and different approaches. For example, row_number() over(order by u.Reputation desc) only works with Transact-SQL language, it would fail with syntax error on every other DBMS that I know of.
The best way to write queries that can port to multiple DBMS is to use a library/framework that supports it. For example, Python has SQLAlchemy and Java has Hibernate. These can adapt queries to the database being used.

If not, what is non-standard? How would I standardize it?

See (1) above. There's not really a "standard" per-se, it really depends on the DBMS and on the application code that uses it.

I don't like all-uppercase queries - I find them more difficult to read, so I used the lowercase, is that problematic?

No, it is not problematic. Just be consistent with whatever style you use.

Is it possible, in a standard way, to factor pieces out of this query? Or nonstandard way specific to SEDE (or other system)? For example, could I avoid doing lower(Location) repeatedly? What about aliasing my joined table outside the code block for usage inside of it? I have order by u.Reputation desc twice, for example, can I factor that out?

The nature of SQL makes it somewhat difficult to refactor parts of queries out. Most SQL engines support the creation of user-defined functions and/or procedures, which can help to simplify queries. In your case those calls to lower() and upper() are needed because SEDE has case-sensitivity enabled.

Review

from -- denormalize the Users with their Badges, i.e., flatten into a single table.
    Users u
  join
    Badges b

This from clause is a bit ugly. The aliases u and b are not very good names. It's a good habit to use meaningful names for aliases, which are meant to be identifiers rather than just a way to shorten the references. Here with having such short table names, an alias seems like overkill. It is also a good habit to use the as keyword and to specify the schema (although in the case of SEDE they are all the same schema, some other databases have tables organized in multiple schemes and it can become ambiguous).
from
    dbo.Users as usr
  -- denormalize the Users with their Badges, i.e., flatten into a single table.
  join dbo.Badges as bdg
    on usr.Id = bdg.UserId

[Rank] would make a better column name than [#].

Your order by clause at the end is redundant, you are already sorting in your row_number() clause, so no need to sort them again. orber by is expensive so you would want to limit the amount of ordering as much as you can.

We can extract the badge name and minimum reputation into variables:
declare @targetBadgeName nvarchar(50) = N'python';
declare @minimumReputation int = 1000;

And just use those in the query. You could also search multiple badges if you used a table variable and then just joined it.
I think it would be elegant to include the Gold, Silver and Bronze into the query if you want that as part of your result set. Note that this will decrease performance due to the min() aggregate and resulting group by clauses:
case min(bdg.Class)
  when 1 then 'Gold'
  when 2 then 'Silver'
  when 3 then 'Bronze'
  else 'unknown' end as [Highest Badge],

Now your result set will look like this:

Rank User Link               Reputation Highest Badge Location
---- ----------------------- ---------- ------------- ---------------------------
1    cletus                  368937     Bronze        New York, NY
2    Triptych                96145      Gold          NYC
3    Claudiu                 81412      Gold          New York, NY
4    Ben Hoffstein           59650      Bronze        New York, NY
5    Yuji 'Tomita' Tomita    59318      Gold          New York, NY
6    danben                  42686      Bronze        New York, NY
7    David Robinson          42489      Gold          New York, NY
8    Larry Lustig            33544      Bronze        New York, NY
9    Aaron Hall              32035      Gold          New York, NY, United States
10   chown                   31348      Silver        New York, NY

Finally, we can use a bit of SEDE magic to parametrize the query:
-- badgeName: Badge to search for:
declare @targetBadgeName nvarchar(50) = N##badgeName:string?python##;
-- minimumRep: Minimum reputation:
declare @minimumReputation int = ##minimumRep:int?1000##;

This will let you search other badges and different rep thresholds.

Everything combined (demo):
-- top users in NYC region for desired tag (default Python)
-- forked from Avinash Raj's query for Chennai

-- badgeName: Badge to search for:
declare @targetBadgeName nvarchar(50) = N##badgeName:string?python##;
-- minimumRep: Minimum reputation:
declare @minimumReputation int = ##minimumRep:int?1000##;

select
    row_number() over(order by usr.Reputation desc) as [Rank],
    usr.Id as [User Link],
    usr.Reputation,
    case min(bdg.Class)
      when 1 then 'Gold'
      when 2 then 'Silver'
      when 3 then 'Bronze'
      else 'unknown' end as [Highest Badge],
    usr.Location
from
    dbo.Users as usr
  -- denormalize the Users with their Badges, i.e., flatten into a single table.
  join dbo.Badges as bdg
    on usr.Id = bdg.UserId
where
    bdg.name = @targetBadgeName
  and
    usr.Reputation >= @minimumReputation
  and
    (
      lower(usr.Location) like '%nyc%'
      or upper(usr.Location) like '%NY, NY%'
      or lower(usr.Location) like '%staten%'
      or lower(usr.Location) like '%bronx%'
      or lower(usr.Location) like '%queens%'
      or lower(usr.Location) like '%new york, ny%'
      or lower(usr.Location) like '%manhattan%'
      or lower(usr.Location) like '%brooklyn%'
    )
group by
  usr.Id,
  usr.Reputation,
  usr.Location;


Organizing classes for Blackjack game I have to write a Blackjack game for college using the Processing language. It has to be done in a strict object-oriented approach, so I can't make an object of a class in another class unless there is some connection.

We have to use a Player interface:

interface Player {

  // This is the interface you are expected to implement for both
  // the computer controlled 'dealer' and the human player(s)

  // You can change the return types for any of the items in the interface.
  // You can also add arguments two the first two method listed below

  void twist(/* you can add an argument here */);               // This method should allow the player to add acard to their hand

  void updateValueOfHand(/* you can add an argument here */);   // Updates the the value of the cards the player has in their hand

  void stick();        // Called when the player does not want to take another card

  void updateScore();  // Updates the player's overall score (number of games they have won)

  int getValueOfHand(); // Getter method for the value of the cards the player has in their hand

  int getScore();      // Getter method for the player's overall score (number of games they have won)

  void showHand();     // Used to display the player's hand on screen

  void clearHand();    // Used at the end of a round to empty the cards in the player's hand

}

I have done a storyboard for the game:

interface Player
class Card
class Deck extends Card

class Computer implements Player
class Human implements Player

I am planning to have a class called ScoreTable where I can keep and show the scores and a Button class where I can draw the buttons.

What kind of classes should I have? Should I have a ScoreTable class which implements Player, and also a standalone Button class?

You are beginning to mix up the business logic with the user interface. You should (be able to) design a "game driver" class without regard to user interface. You want to keep your GUI code separate from your game logic code. And another OO paradigm is to create new objects separately and pass them into where they are used.

I see a Game class ( "Dealer" might be a better name) that handles the cards, deals them to players, etc. Then your ScoreTable class wires-up it's buttons to the Game methods. For example a "Deal" button's click event handler might be Game.DealCards().

pseudo code for the idea:

  Player me = new Computer();
  Player you = new Human();
  Game aNewGame = new Game (me, you);
    // I'm imagining that the game creates the card deck internally
  ScoreTable theCasino = new ScoreTable(aNewGame);

  // inside ScoreTable constructor...
  DealButton.onClick += aNewGame.DealCard();

Suggestions

Classes and their interaction reflect real life. Write code that way. I.E. A "Dealer" deals "Card"s from a "CardDeck" to "Player"s. (Now I really like "Dealer" better than "Game" as a class name). Therefore the Game must have (references to the) Players, so we pass player objects into the Game constructor.
Think in terms of the ScoreTable only displaying the game, NOT actually playing the game. The Game class plays the game. Thus the ScoreTable must interact/communicate with Game to display what's going on.
Think of ScoreTable as telling Game what to do and Game does it. For example you click "Hit Me" (ScoreTable) and that fires the Game.DealCard() method.
BlackJackTable sounds better to me than ScoreTable for that class name.

  Can i have a ScoreTable class which implements Player and a stand alone Button class?

Do not do this. Think in real-life terms to keep things straight. A ScoreTable is a Player? That does not make sense. A ScoreTable has to display stuff on the screen so it is (inherits from) a Window - whatever the appropriate class is in Java.

  What kind of classes should i have?

I see a ScoreBoard class. Your diagram inspired the thought. The diagram directly suggests the class would have an array (of hands) each element of which holds the final score for both players.  This data structure, then, maps clearly to your UI concept. I see it as a separate class because it keeps score for both players, so it is inappropriate to be inside the (individual) Player class. A Player might have his score (hand) for the current game, but not past games, and certainly not the past games of a different player.

*Your Other classes *

Card
Deck - it's an stack of cards, it is not a card. And I mean "stack" in the way it behaves.
Human - a Player that is stupid (we're going to click buttons to tell her what to do).
Computer - a Player that knows by itself when to hold 'em and when to fold 'em
Game - the dealer essentially. Shuffles and deals cards.
ScoreTable - displays what's going on. "Controls" the Game via button clicking, etc.
ScoreCard - keeps history of past game results.


Repetitive unit tests for a programming language I've got a recreational regex-based programming language called Retina. Before the next big refactoring of the interpreter, I want to rework how I test the code. The interpreter class works such that it's instantiated once with the program (provided as a list of strings, which usually but not necessarily are all single lines) and can then be run several times on different inputs.

I'd like to make use of this in the new test suite so that I can specify tests by defining a program once and then testing a bunch of input/output pairs on it. Virtually all tests will be written this way, and there will be a lot of them, so I'd like to be able to set this up with as little syntactic overhead as possible which might distract from the programs and I/O pairs. Here's what I'm currently doing. I've got a base class for my test cases:

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Retina;
using System.Collections.Generic;
using System.IO;
using System.Linq;

namespace RetinaTest
{
    [TestClass]
    public class RetinaTestBase
    {
        protected void AssertProgram(List<string> sources, List<(string input, string output)> ioPairs)
        {
            var interpreter = new Interpreter(sources);

            foreach ((var input, var expectedOutput) in ioPairs)
            {
                var actualOutput = new StringWriter();
                interpreter.Execute(input, actualOutput);

                Assert.AreEqual(expectedOutput, actualOutput.ToString());
            }
        }
    }
}

In the derived test classes I can then simply do:

namespace RetinaTest
{
    [TestClass]
    public class ReplaceStageTest : RetinaTestBase
    {
        [TestMethod]
        public void TestBasicReplacement()
        {
            AssertProgram(
                new List<string> {
                    "a",
                    "x"
                },
                new List<(string, string)> {
                    ( "abc", "xbc" ),
                    ( "Hello, World!", "Hello, World!" ),
                    ( "banana", "bxnxnx" )
                }
            );
            /* etc */
        }
    }
}

A List of string tuples seemed like the natural type for the I/O pairs, but I find the constructor is almost too much in terms of overhead. I'm tempted to turn them both into arrays and let the compiler infer the type with new[], or even provide the I/O pairs as an even-length variadic argument list. That seems like bad style to me though because I could easily mess up the pairing if I forget a string somewhere. Not using C# very regularly I'm also not too clear on the exact benefits and shortcomings of the various list types (in this case mainly lists and plain arrays).

Is there a way to set this up which is both idiomatic and requires very little visual overhead beyond specifying a program (as a list of strings) and a list of I/O pairs? I'd be happy about any other criticism of the code as well, of course.

You can make your code less verbose and more expressive at the same time. Now let me explain what I mean.

Make creating test cases easier without all those new lists and types like this

AssertProgram(new TestData
{
    Sources = { "a", "x" },
    TestCases =
    {
        { "abc", "xbc" },
        { "Hello, World!", "Hello, World!" },
    }
});

It's at the same time less verbose because there is only one new for TestData and it's easier to understand what all the strings are.

In order for this to work you'll need three helpers with lists that are already instantiated so that you can just use the collection initializer.

class TestData
{
    public List<string> Sources { get; set; } = new List<string>();
    public TestCaseList TestCases { get; set; } = new TestCaseList();
}

class TestCase
{
    public string Input { get; set; }
    public string Output { get; set; }
}

class TestCaseList : List<TestCase>
{
    public void Add(string input, string output) => Add(new TestCase { Input = input, Output = output });
}

The last class is necessary so that you can add a new Add overload to the list and create your test case easily.

protected void AssertProgram(TestData testData)
{
    var interpreter = new Interpreter(testData.Sources);

    foreach (var testCase in testData.TestCases)
    {
        var actualOutput = new StringWriter();
        interpreter.Execute(testCase.Input, actualOutput);

        Assert.AreEqual(expectedOutput, actualOutput.ToString());
    }
}

If we now rewrite the AssertProgram method to work with the TestData class, it'll be much easier to use the properties than simple strings or repeat the tuple names everywhere.

BTW: you are not using the Output anywhere... yet?


Isometric Projection and Culling I'm working on a small isometric drawing system in JavaScript. After much help from the Gamedev section, reading old questions, I've got something working. However, it's still a little hacky. I need to fix the culling to be more straightforward and it still isn't quite getting edge tiles down correctly.

I want to improve the code to ensure that:

The "center" tile is actually centered in the drawing area.
The are always enough edge tiles.
The code is straight-forward

Link to live Codepen.

Here is the camera drawing function:

function cameraDraw(position)
{
  var cen = {
        a: position.x + position.y,
        b: position.x - position.y
      },
      max = { //Use testSize instead of canvas size so we can see overdraw
        a: Math.floor(testSize.y/(terrainImg.width/2))+1, //Adding +1 to each for better edging?? Is this normal?
        b: Math.floor(testSize.x/terrainImg.width)+1
      },
      startPos = {
        x: (((cen.a - max.a) + (cen.b - max.b))/2),
        y: (((cen.a - max.a) - (cen.b - max.b))/2)
      },
      offset = { //Subtract 100 to show overdraw
        x: (startPos.x - startPos.y) * (terrainImg.width/2) + terrainImg.width/2 + terrainImg.width/4 - 100, //+terrainImg widths for better placement
        y: (startPos.x + startPos.y) * (terrainImg.width/4) + terrainImg.width/4 - 100
      };

  for(var a = cen.a - max.a; a < cen.a + max.a; a++)
  {
    for(var b = cen.b - max.b; b < cen.b + max.b+1/*I add +1 here to take care of edge issue on the bottom...Should I have to do that, if I'd done it right, above?*/; b++)
    {
      if ((b&1) != (a&1))
        continue;

      var x = (a+b)/2,
          y = (a-b)/2;

      world[x][y].draw(offset, position);
    }
  }
}

And here is the tile .draw function:

this.draw = function(offset, p)
{
  var drawPos = {
    x: (X-Y)*(terrainImg.width/2) - offset.x,
    y: (X+Y)*(terrainImg.width/4) - offset.y
  };

  if(X == p.x && Y == p.y)
    drawPos.y -= 15;

  ctx.drawImage(terrainImg, drawPos.x, drawPos.y);
};

This code is pretty well written, although there are a few things that really bug me a little.

First off, inline comments, like the one you've positioned inside that for loop, are eyesores to look at. Preferably, comments should stay on their own separate lines.

Secondly, your naming could be improved. For example, what does the variable cen do? It's name doesn't give any hint as to what it does, nor do it's contained items, a and b. Variable names should be descriptive, and describe the purpose of a variable, all while not being too long, or too short.

Finally, use braces around your if statements. Have you ever heard of the "goto fail" bug? That's what happened in a bug with Apple SSL. The error, in question, looked something like this:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;  /* MISTAKE! THIS LINE SHOULD NOT BE HERE */

Just use braces. They'll prevent all sorts of mistakes from occurring, and they'll make things look clearer overall as well.


Calculate weekly net pay My task is:

  Write a program that requests the hours worked in a week and then
  prints the gross pay, the taxes, and the net pay. Assume the
  following:

  Basic pay rate = $10.00/hr
  Overtime (in excess of 40 hours) = time and a half
  Tax rate: 15% of the first $300

  20% of the next $150
  25% of the rest

Is my code way too long and complicated for that?

#include <stdio.h>
#define PAYRATE 10 //basic pay rate per hour
#define OVERTIME 15 //in excess of 40 hours a week
int NetPay(int hours);

int main()

{
    int userWeeklyHours;

    printf("Please enter your total weekly working hours: \n");
    scanf("%d", &userWeeklyHours);// get the user weekly hours
    NetPay(userWeeklyHours);

    return 0;
}

int NetPay(int hours)// implementing the function to calculate total pay, total taxes, and net pay

{
    int firstRate, secondRate, restOfRate, secondAmount, rest, payAfterTax, payedBeforeTax, overHours;

    if (hours > 40)
    {
        overHours = hours - 40;
        payedBeforeTax = (40 * PAYRATE) + (overHours * OVERTIME);
    }

    else
        payedBeforeTax = hours * PAYRATE;//defining the user first paycheck before taxes

    if (payedBeforeTax <= 300)//paying only first rate case
    {
        firstRate = payedBeforeTax*0.15;
        payAfterTax =payedBeforeTax - firstRate;
        printf("your total gross is %d, your taxes to pay are %d, your net pay is %d", payedBeforeTax, firstRate, payAfterTax);
    }

    else if (payedBeforeTax > 300 && payedBeforeTax <= 450)//paying first and second rate
    {
        secondAmount = payedBeforeTax - 300;
        firstRate = (payedBeforeTax - secondAmount) * 0.15;
        secondRate = secondAmount * 0.20;
        payAfterTax = payedBeforeTax - (firstRate + secondRate);
        printf("your total gross is %d, your taxes to pay are %d, your net pay is %d", payedBeforeTax, firstRate + secondRate, payAfterTax);

    }

    else if (payedBeforeTax > 450)// paying all rates
    {
        rest = payedBeforeTax - 450;
        secondAmount = (payedBeforeTax - 300) - rest;
        firstRate = (payedBeforeTax - (rest + secondAmount)) * 0.15;
        secondRate = secondAmount * 0.20;
        restOfRate = rest * 0.25;
        payAfterTax = payedBeforeTax - (firstRate + secondRate + restOfRate);
        printf("your total gross is %d, your taxes to pay are %d, your net pay is %d", payedBeforeTax, firstRate + secondRate + restOfRate, payAfterTax);

    }

    return payAfterTax;
}

I'd start extracting some more functions.

What about decomposing

int NetPay(int hours){
    int payedBeforeTaxes = payedBeforeTaxes(hours);
    return applyTaxes(payedBeforeTaxes);
}

int payedBeforeTaxes(hours){
    return regularPay(hours) + overtimePay(hours);
}

int applyTaxes(int amountBeforeTaxes){
    int firstRateTaxes  = applyFirstRate(amountBeforeTaxes);
    int secondRateTaxes = applySecondRate(amountBeforeTaxes);
    int thirdRateTaxes  = applyThirdRate(amountBeforeTaxes);
    return amountBeforeTaxes - firstRateTaxes -
           secondRateTaxes - thirdRateTaxes;
}

At this point you just need to implement the functions with the appropriate code.
You'll notice that with this structure each function only does one thing and it will be much simpler and clearer.

Alternatively you can replace your apply taxes introducing an array of `(rate, threshold).
You will have to loop through the array, and at each step compute the amount you have to apply that tax rate and the associated tax amount.


A little integer parser vs clean code I was said to post this as a new question. I improved my code from:

Little part of the string to integer parser vs Clean Code

However I think that it break SRP as it parses, iterates through string and checks if letters are numeric. Could someone involved in OOP say what does he think of it? I agree that wrapping Index in class was a mistake, but it lost modularity in my opinion. I also see duplication in two IntegerParser's methods however I'm not sure how to remove it. In the end I think this is much less open for adding new functionalities.

Main.java

public static void main(String[] Args)
{
        IntegerParser intparser = new IntegerParser();
        System.out.println(intparser.getFirstNumber( "lolo543" ));
}

IntegerParser.java

package intparser;

public class IntegerParser
{
    private String parsingText;
    private int startIndex;
    private int lastIndex;

    public int getFirstNumber(  String textToParse )
    {
        parsingText = textToParse;
        startIndex = getIndexOfFirstNumericChain(0);
        lastIndex = getIndexOfLastNumberInChain(startIndex);
        if ( startIndex == -1 )
            return -1;
        if ( lastIndex == -1 )
            lastIndex = parsingText.length();
        return Integer.parseInt( parsingText.substring(startIndex, lastIndex) );
    }

    private int getIndexOfFirstNumericChain( int index )
    {
        if ( index < 0 ) return -1;
        for ( int i = index; i < parsingText.length() ; i++ )
            if (numberAt(i)) return i;
        return -1;
    }

    private int getIndexOfLastNumberInChain( int index )
    {
        if ( index < 0 ) return -1;
        for ( int i = index; i < parsingText.length() ; i++ )
            if (!numberAt(i)) return i;
        return -1;
    }

    private boolean numberAt( int index )
    {
        char letter = parsingText.charAt(index);
        if (  letter >= '0' && letter <= '9' )
            return true;
        return false;
    }
}

Prologue

To be frank, this is a sub-optimal problem to be applying object-oriented programming concepts, for two reasons that I can think of:

It is only a trivial step to extract the first subsequence of digits from a String, and
Java has a number of built-in methods to do that.

Object-oriented programming concepts are more applicable to richer classes ("models") with appropriately-scoped actions, in layman terms. For example, you may want to drive(Direction.FORWARD) a Car, make a Duck quack() or process(Message) messages using a MessageProcessor.

Learning the APIs, or the Alternative Approach

Before any attempts to write new code, you often need to understand what are some of the existing solutions to the problem at hand. This implies you need to be reasonably familiar with the provided features/APIs from your programming language of choice, and subsequently third-party libraries.

Using Java as an example, unless it's for self-learning purposes, I will not expect to see new and extremely similar variations of a List implementation for every codebase or project I come across: I simply expect the use of the ArrayList objects to do that.

So, conveniently, what are the some of the other available solutions for this? Regex, Pattern and Matcher come into play:

public static int getFirstNumber(CharSequence input) {
    Matcher matcher = Pattern.compile("[0-9]+").matcher(input);
    return matcher.find() ? Integer.parseInt(matcher.group()) : -1;
}

What is being done here:

The pattern "[0-9]+" (or "\\d+") represents a subsequence of digits we want to find, which must at least be a digit long.

The surrounding parentheses specifies that we want to reference our matches using grouping: Following clarification by @Boris the spider.
The Pattern object here represents the regex pattern, and certain interactions it can have with String (or more accurately, CharSequence) inputs.

We then pass our input to matcher() to create our Matcher object.

The Matcher object here represents the results of applying the regex pattern on the input.

We then 'ask' by matcher.find() to see if we have matched any subsequences.

If we do, we can get our first match by matcher.group(), else we return -1 as you have used as such.
edit: As suggested by @ Machinarius, you may consider throwing an Exception to indicate that no subsequences were found.

You want new features? Sure! Matcher has other methods that lets you process on the input iteratively. For example, you can use it to iterate till the last subsequence, you can combine all hits into a larger number, etc. It is not as restrictive as you might think.

Hopefully, this shows you why you do not have to keep writing new code every time.

Reinventing the wheel?

Now, suppose you do not wish to rely on regex for this problem. What are some of the improvements you can use for your solution?

You should make your methods static, as it is generally not a recommended approach to keep creating wrapper objects for 'one-off' operations.

Instead of storing textToParse as a class-level field, pass it between your methods.

You should only look for the last index if startIndex is not -1. It's nice that you do a bit of input validation here, but that becomes redundant if you structure your method calls correctly.
The return statement from numberAt(int) should just be return letter >= '0' && letter <= '9', since it's redundant to use an if statement here.
As far as my understanding goes, there isn't a clean, short and simple way to de-duplicate the two loops you are running across the getIndexOf... methods. Well, I can think of a slightly more fluent way using Java 8's IntStream and IntPredicate but I'll defer that suggestion for the time being, partly also because I don't think it's a good fit.
Consistent bracing, this can be a pet peeve of some developers... Java's recommended way of using braces for methods (and if statements) is actually in my alternative solution above.

Unit testing

Last but not least, one of my more favorable topics here... It is not good enough to just display the output of your method. You need to perform assertions on the output to ensure your method is working as it should. This is where unit testing frameworks like JUnit, TestNG and Hamcrest matchers come into play to facilitate the writing of unit testing code. As a quick illustration, you should have a something similar to the snippet below that can be run as a unit test. Somewhat conveniently, both JUnit and TestNG are using their respective @Test annotations, so it wouldn't make a difference here. assertThat() and equalTo() are from the Hamcrest library.

public class IntegerParserTest {
    @Test
    public void doTest() {
        assertThat(Integer.valueOf(IntegerParser.getFirstNumber("abc123xyz456")),
            equalTo(Integer.valueOf(123)));
    }
}


Generating Norwegian social security numbers I have written some code in Node.js that takes a date as argument and return all the possible combination of social security numbers.

I'd really appreciate some advice when it comes to these areas:

Coding style
Code structure

In my code I have included a brief description on how these numbers are generated.

personnr.js

/*
Norwegian social security-number structure;

dd mm yy i1 i2 i3 k1 k2

i1, i2, i3 => 'Individ'-numbers

This structure is used pr. oct. 2013;

000–499 for people born some time in 1900–1999.
500–999 for people born some time in 2000–2039.

k1, k2 => calculated based on the numbers above (a checksum)

more info;

https://no.wikipedia.org/wiki/Fødselsnummer
*/

module.exports = init;

function Personnr(birthday) {
    if (typeof birthday !== 'string')   throw new Error('Argument must be a string');
    if (birthday.length !== 8)          throw new Error('Argument has to be on the format \'DDMMYYYY\'')

    var dd      = addZero(birthday, 0, 2),
        mm      = addZero(birthday, 2, 4),
        yy      = addZero(birthday, 6, 8),
        yyyy    = addZero(birthday, 4, 8)

    this.birthday   = [dd, mm, yy].join('');
    this.dd         = dd;
    this.mm         = mm;
    this.yy         = yy;
    this.yyyy       = yyyy;

    this.individ = {
        i1: 0,
        i2: 0,
        i3: 0
    };
}

Personnr.prototype.make = function() {
    var range       = getRange(this.yyyy),
        min         = range[0],
        max         = range[1],
        generate    = generateIndividSiffer(min, this.yy, this.individ),
        result      = [];

    for(var i = min; i < max; i++) {
        var individ = generate();
        var kontroll = generateKontrollSiffer(this.dd, this.mm, this.yy, this.individ);

        var personnr = [this.birthday, individ, kontroll].join('');
        result.push(personnr);
    }

    return result;
}

function init(birthday) {
    return new Personnr(birthday);
}

//Private functions

//If the number is < 10, an extra '0' is inserted before (e.g. 08, 09 etc..)

function addZero(number, min, max) {
    if (number.length === 2) {
        var number = parseInt(number.slice(min, max)),
            result = ('0' + number).slice(-2);
    }

    return result || number.slice(min, max);
}

function getRange(year) {
    var min = null,
        max = null;

    if (year >= 1900 && year <= 1999) {
        min = 0;
        max = 499;
    } else if (year >= 2000 && year <= 2039) {
        min = 500;
        max = 999;
    }

    return [min, max];
}

function generateIndividSiffer(start, year, individ) {
    var increment = start;

    return function() {
        increment++;

        individ.i3 = increment % 10;

        if (individ < 100){
            individ.i1 = 0;
            individ.i2 = parseInt(increment / 10) % 10;
        } else if (increment < 10) {
            individ.i1 = increment.i2 = 0;
        } else {
            individ.i1 = parseInt(increment / 100) % 10;
            individ.i2 = parseInt(increment / 10) % 10;
        }

        return [individ.i1, individ.i2, individ.i3].join('');
    }
}

function generateKontrollSiffer(day, month, year, individ) {

    var d1  = parseInt(day / 10),
        d2  = day % 10,

        m1  = parseInt(month / 10),
        m2  = month % 10,

        y1  = parseInt(year / 10),
        y2  = year % 10,

        i1  = individ.i1,
        i2  = individ.i2,
        i3  = individ.i3;

    var k1 = 11 - ((3*d1 + 7*d2 + 6*m1 + 1*m2 + 8*y1 + 9*y2 + 4*i1 + 5*i2 + 2*i3) % 11);
    if (k1 > 9) k1 = 0;

    var k2 = 11 - ((5*d1 + 4*d2 + 3*m1 + 2*m2 + 7*y1 + 6*y2 + 5*i1 + 4*i2 + 3*i3 + 2*k1) % 11);
    if (k2 > 9) k2 = 0;

    return [k1, k2].join('');

}

test.js

var personnr = require('./personnr');

var results = personnr('16111992').make();
console.log(results);

Here are some general critique on the code. I'm not that familar with the specifics of javascript so I'll leave that to people who knows this better.

Validate input

When creating the object you should validate the brithday making sure it's correct. This can be done by adding a check_birthday function like e.g.

function check_birthday(day, month, year){

   // Check if month is in [1,12]
   if(month < 1 || month > 12) return false;

   // Check if day is valid given the month (i.e. < 31, 30 (29 if leap year), 31, ...

   // Check that year is valid (get_range assumes [1900,2039])
   if(year < 1900 || year > 2039) return false;

   return true;
}

See this page for how the day-check can be implemented (taking leaps-years into account). Another, option is to take the input of the function to be a birthday string instead.

Functions should do what the name says it does

The addZero function (as it's used) looks useless / does not really do what it says it does. For example for dd you send in a string ddmmyyyy, extract the [0,2] part of the string giving us dd, parse that as an int, add a zero if the number is less than 10 and then return it as a string. This should be equivalent of just extracting the dd part (given that the string is a valid birthday-string) thus the only things this function really does is to extract the [min,max] part of the string and does not add any zeros.

I would do the splitting of birthday into dd, mm and yyyy outside of addZero, i.e. in the initialization.

dd = birthday.slice(0, 2);

If you need an addZero function I would make this more generic (which allows it to be used for other stuff if needed) and make sure it does exactly what the name suggest: it should take in a string/number and output a string of a given size with leading zero(s) added. Here is an example of how one can do this:

function addLeadingZeros(number, length){
   var paddednumber = '' + number;

   // Here we just return number / show error message that length
   // of number is larger than the required length
   if(number.length > length) return paddednumber;

   // Pad number with zeros
   while (paddednumber.length < length) {
     paddednumber = '0' + paddednumber;
   }
   return paddednumber;
}

Other things to consider

As pointed out by Fiksdal, the third control digit i3 gives the sex of the person. If you are interested in separating between males and females when making the allowed numbers then the sex should be part of the class which can be set by changing personnr(number) to personnr(number, sex) adding sex to the individ struct and in make you need to add a check for this if(individ.i3 % 2 == individ.sex) result.push(personnr); (here for simplicity assuming sex is stored as 0 for women and 1 for men).

Right now you store the day, month and year as a string. This might be good for the intended use, but I would also consider making these into integers as this just sounds more natural to me. If you also need them as a string then this can easily be done with a addZero function.


PHP santization of textarea input My application will accept textarea content that is submitted by a user, and i would like some people to review my code to make sure there is no security vulnerability such as XSS.

My mySQL column that will save this information is a column of type TEXT and is not required and nullable.

When storing the data to database, my script is doing the following:

// to avoid inserting html tags in the database
$input = str_replace(["<", ">"],"", $_POST['userinput'])

// to avoid saving problematic characters such as quotes
$cleanInput = htmlentities(input , ENT_QUOTES)

// store the content
$addPostStmt = $conn -> prepare("
  INSERT INTO posts(description) VALUES ( ?)
");

$addPostStmt -> bind_param("s", $cleanInput);
$addPostStmtExecute = $addPostStmt -> execute();

When presenting the data to the user, the script is doing the following:

<?php echo htmlspecialchars(html_entity_decode($post['description']), ENT_SUBSTITUTE); ?>

No, don't do that. You seem to be filtering and escaping values out of paranoia rather than understanding what exactly would lead to a vulnerability. As a result, you are corrupting your data.

A well designed application should use the database to store the value that the user typed into the textarea, not some mangled representation of it. If you mangle the data like that before storing it, then:

Certain characters that the user typed get dropped. (What if the user input is x + 3 < 5? The data would no longer make sense after you drop the < character.)
Your database is not reliably searchable. (What if the user input is She said "yes!"? Then you would store a value in the database with &quot; in it.)
If you arbitrarily apply escaping to string just in case, then you'll have a hard time keeping track of how to unescape it correctly when regurgitating the data. (This often leads to bugs where the user sees garbage like his &amp; hers, or even worse, his &amp;amp; hers.)

What's the right way? Don't mangle the data; just store it faithfully:

// store the content
$addPostStmt = $conn -> prepare("
  INSERT INTO posts(description) VALUES (?)
");

$addPostStmt -> bind_param("s", $_POST['userinput']);
$addPostStmtExecute = $addPostStmt -> execute();

When outputting the data as HTML, apply HTML escaping:

Description:	<?php echo htmlspecialchars($description); ?>


Method to get user information (OOP / MVC approach) I coded a small user script with PHP. I'm improving the script, learning from my mistakes. I recently learned about object oriented programming and wrote 2 big god classes (user class and message class). That didn't feel right, so read about the MVC model. I wrote a controller class to handle the big god classes while being easy to use in the views (I want to be able to rewrite the big god classes without rewriting the views).

This controller class allows me to streamline my big god classes, which is exactly what I wanted to achieve.

I'm replacing the method user_search() with user_search_info(). It should return a row of user information based on the given user_id or user_id. It should return false if the query fails or returns empty. When I am satisfied, I will delete user_search().

The new method works like I want it to, and I find it very elegant and easy to read. But is it the right approach? I'm sure there's still room for improvement.

This is from the user class file:

  class user{
    // new methods don't care about this anymore so this will get deleted
    public $logs = array();

    // ... irrelevant methods left out ...

    // old method
    public function user_search($db_connection, $user_id=null, $username=null){
      $result = false;
      if($user_id == null && $username == null)
        $this->logs[] = "No information to search on has been found.";
      elseif($db_connection == false)
        $this->logs[] = "Something went wrong with the database.";
      else{
        $this->logs[] = "Searching the user in the database.";
        $query_user_search = $db_connection->prepare('SELECT * FROM users WHERE user_username=:user_username OR user_id=:user_id');
        $query_user_search->bindValue(':user_username', $username, PDO::PARAM_STR);
        $query_user_search->bindValue(':user_id', $user_id, PDO::PARAM_STR);
        $query_user_search->execute();
        if($result = $query_user_search->fetch())
          $this->logs[] = "We found the user.";
        else
          $this->logs[] = "We didn't find the user.";
      }
      return $result;
    }

    // new method
    public function user_search_info($db_con, $user_id=null, $user_username=null){
      $query_user_search_info = $db_con->prepare('SELECT * FROM users WHERE user_username=:user_username OR user_id=:user_id');
      $query_user_search_info->bindValue(':user_id', $user_id, PDO::PARAM_STR);
      $query_user_search_info->bindValue(':user_username', $user_username, PDO::PARAM_STR);
      $query_user_search_info->execute();
      return $query_user_search_info->fetch();
    }
  }

And this is from the controller class file:

public function return_user_search_info($search_user_variable, $search_user_value)
{
  $user_search_info = false;
  if($search_user_variable == "user_id")
    $user_search_info = $this->user->user_search_info($this->db_con, $search_user_value, "");
  elseif($search_user_variable == "user_username")
    $user_search_info = $this->user->user_search_info($this->db_con, "", $search_user_value);
  return $user_search_info;
}

What you have isn't really a function to search for a user, but a function to get a user, either by id or by name.

So instead of having one function that does two different things, why not have two functions?

public function getUserById($db_con, $id){
    $queryGetUser = $db_con->prepare('SELECT * FROM users WHERE user_id=:id');
    [...]
}

public function getUserByUsername($db_con, $username){
    $queryGetUser = $db_con->prepare('SELECT * FROM users WHERE user_username=:username');
    [...]
}

Now you don't have to pass empty strings to the function (which is really confusing to use), and the functions have a proper name as well.


Reversing a string - two approaches Test Case:
Input- "I'm hungry!"
Output- "!yrgnuh m'I"

Approach 1: In this approach, I used a empty string and bind it with the input string reversely.

public static class ReverseString {

   public static string Reverse (string input) {

    //bind the string to an empty string reversly
    var reversedString = "";

    //check if the input is empty
    if (input == "")
    {
        return "";
    }
    else
    {
        for (int i = input.Length - 1; i >= 0; i--)
        {
            reversedString += input[i];
        }
       return reversedString;
    }
  }
}

Approach 2: In this approach, I've created an empty char array which has the same length of the string. Then I've copied the value of string's last index to char array's first index and so on.

public static class ReverseString {
   public static string Reverse (string input) {

      char[] chars = new char[input.Length];

      for(int i = 0, j = input.Length -1; i <= j; i++, j--)
      {
        chars[i] = input[j];
        chars[j] = input[i];
      }

      return new string(chars);
   }
}

There are lots of approaches like this(without using built in library). But I wonder which one is the most recommended among programmers preferably C# programmers.Which one do you recommend and why?

Clarity of Approaches

With the first approach, I look at it and I can tell straight away that you are reversing a string.  With the second approach, I need to study it for a minute or two to work out what you're doing.

Unnecessary Code

In the first approach, the check for an empty string is not necessary.  In this case, your logic will not even enter the for loop, resulting in an empty string being returned anyway.

Performance

As you may know, strings are immutable objects in .Net.  It is good practice to use a StringBuilder to create strings in this way, like so:

var reversedString = new StringBuilder(input.Length);
for (int i = input.Length - 1; i >= 0; i--)
{
    reversedString.Append(input[i]);
}
return reversedString.ToString();


Generating combinations of n elements in groups of k I've written this program that writes all the combinations (without repetition) of n elements in groups of k.

I think the code is good, but I like to know if you have some better (or faster) solutions.

The elements to combine are always the number from 0 to n-1, the program calls a function (newCombo()) for each generated combination.

EG: Using n=6 and k=4 the program generates the following output:

0 1 2 3
0 1 2 4
0 1 2 5
0 1 3 4
0 1 3 5
0 1 4 5
0 2 3 4
0 2 3 5
0 2 4 5
0 3 4 5
1 2 3 4
1 2 3 5
1 2 4 5
1 3 4 5
2 3 4 5

The code is below:

#include <stdio.h>

void newCombo(int *a,int n,int k);

void newCombo(int *a,int n,int k)
{
    int i;
    for(i=0;i<k;i++)
        printf("%d ",a[i]);
    puts("");
}

int main(int argc,char *argv[])
{
    int a[30];
    int n,k,p;

    do {
        printf("Insert n and k - EG: 5 3: ");
        if (scanf("%d %d",&n,&k)!=2)
            return 1;
        if (k>n)
            puts("k shall be <= n !");
        if (k==0 || n==0)
            puts("k and n shall not be 0!");
        if (k>30)
            puts("k shall be <=30");
    } while (k>n || k==0 || n==0 || k>30);

    p=0;
    a[0]=-1;
    do {
        if (++a[p]>n-k+p) {
            p--;
        } else {
            if (p<k-1) {
                a[p+1]=a[p];
                p++;
            } else {
                newCombo(a,n,k);
            }
        }
    } while(p>=0);

    return 0;
}

Main loop could be simplified a little

There's not too much to say about the current program.  It works and it seems like it is using the fastest method possible.  It basically adds one to the last array element and moves left when an "overflow" happens.  At the end of handling all the "overflows", it moves back to the right, resetting each digit.

The one thing that I would change is basically that when you are moving back to the right, there's no need to use the main loop to do that.  It causes extra compares of (p >= 0) which are always going to be true and extra ++a[p] which are unnecessary.  You could instead handle moving to the right in its own nested loop.  Here's how it would look like:

do {
    if (++a[p]>n-k+p) {
        // Overflow, move to the next element on the left.
        p--;
        continue;
    }
    // Resolve overflows by resetting digits to the right.
    while (p < k-1) {
        a[p+1]=a[p]+1;
        p++;
    }
    newCombo(a,n,k);
} while(p>=0);

An interesting challenge

What if I asked you to write a program that prints out the N'th combination instead of all of them.  In your given example with n=6 and k=4, the 0th combination would be 0 1 2 3 and the 14th (and last) combination would be 2 3 4 5.  I find this task to be much more difficult (and useful) than generating all combinations.


JavaScript function for the get the formatted date of tomorrow I need a JavaScript-function which always computes the date of the next day. Then returns the date as a string, formatted in the way: dd.mm.yyyy
I wrote this code:

const getDateStringForTomorrow = () => {
    const millisOfDay = 1000 * 60 * 60 * 24;
    const oTomorrow = new Date(Date.now() + millisOfDay);
    const day = ("0" + oTomorrow.getDate()).slice(-2);
    const month = ("0" + (oTomorrow.getMonth() + 1)).slice(-2);
    const year = oTomorrow.getFullYear();

    return `${day}.${month}.${year}`;
};

console.log(getDateStringForTomorrow());

Can I expect my function to work as expected and to provide correct results?
What't your opinion about the way I have written the function? To you think it's overly verbose?

With Intl.DateTimeFormat you can also format the date elements to 2 and 4 digits. You code would then look something like this:
const getDateStringForTomorrow = () => {
  const tomorrow = new Date();
  tomorrow.setDate(tomorrow.getDate() + 1);
  const day   = new Intl.DateTimeFormat('en', { day: '2-digit' }).format(tomorrow);
  const month = new Intl.DateTimeFormat('en', { month: '2-digit' }).format(tomorrow);
  const year  = new Intl.DateTimeFormat('en', { year: 'numeric' }).format(tomorrow);
  return day + '.' + month + '.' + year;

};

console.log(getDateStringForTomorrow());

Another variant uses the European date format to get the order of the elements correctly and then only replaces dashes by dots:
const getDateStringForTomorrow = () => {
  const tomorrowDate = new Date();
  tomorrowDate.setDate(tomorrowDate.getDate() + 1);
  const tomorrowStr = new Intl.DateTimeFormat('uk', { day: '2-digit',  month: '2-digit', year: 'numeric' }).format(tomorrow);
  return tomorrowStr.replaceAll('-','.');

};

console.log(getDateStringForTomorrow());

The advantage of using Intl.DateTimeFormat() is that it writes out very clearly what you want.


Refactor c# comparision/filter logic I have to write a comparison logic.

Following is the code to refactor (as it is looking bad).

  // 1. item has filter criteria
  // 2. return true if current matches filter criteria
  // 3. Criteria -> 3 fields should be matched ( Security, OrderType, CounterType)
  // 4. Criteria -> User can specify All (*), If any matching field is All, (NONE for enum), only that field should not be matched, other should be matched
  private static bool CompareOrderItem(Item item, Item current)
{
    String all="All";
    bool flag = true;

    if (!( String.Equals(item.Security, all) ||String.Equals(current.Security, item.Security)))
        flag = false;

    if (!( item.OrderType == OrderTypeEnum.None ||Enum.Equals(current.OrderType, item.OrderType)))
        flag = false;

     if (!( String.Equals(item.Counterparty, all) || String.Equals(current.Counterparty, item.Counterparty)))
        flag = false;

    return flag  ;
}

This is a subjective opinion, but I find it useful to extract methods for the three types of matches you are checking in your code.

I refactored your code and extracted the methods:

bool OrderPartyMatches(Item item, Item current)
bool OrderTypeMatches(Item item, Item current)
bool SecurityMatches(Item item, Item current)

While all I did was move stuff around, I think this version gains in readability, because I find

OrderPartyMatches(item, current)

to reveal its intent more clearly than

( String.Equals(item.Security, all) ||String.Equals(current.Security, item.Security)

which is now encapsulated in this named method.

The final code I ended up with is:

const string all = "All";

private static bool CompareOrderItem(Item item, Item current)
{
    return
        SecurityMatches(item, current) &&
        OrderTypeMatches(item, current) &&
        OrderPartyMatches(item, current);
}

private static bool SecurityMatches(Item item, Item current)
{
    return
        String.Equals(item.Security, all) ||
        String.Equals(current.Security, item.Security);
}

private static bool OrderTypeMatches(Item item, Item current)
{
    return
        item.OrderType == OrderTypeEnum.None ||
        Equals(current.OrderType, item.OrderType);
}

private static bool OrderPartyMatches(Item item, Item current)
{
    return
        String.Equals(item.Counterparty, all) ||
        String.Equals(current.Counterparty, item.Counterparty);
}

Please note how the need for comments disappears, as the code now explains itself progressively.

Some people may only be interested to see that three fields are matched (and thus only look at the first method). Others may also be interested to see how the matching is done, and the explanation is just a few lines away.


Anemic class that binds functionality I have cleanly separated functionality of a message bus - namely the diagnostics (optional), sending, decoding and the bus driver, I need to now bind this together and have done so with a MessageBus class.
It feels anemic though, with a lot of pass through methods.
On the flip side it is necessary for coordinating StartAsync() which links dependencies on the driver and receiver (binding them so they can call back) - I saw no other way of achieving this if I registered all individual interfaces via an inversion of control container as it would get out of sync (how to bind the driver to the decoder for instance).
If this is the wrong place for comments on code please let me know and I will remove straight away. Any pointers much appreciated.
public class MessageBus : IDisposable
{
    private readonly MessageBusReceiver receiver;
    private readonly MessageBusPublisher publisher;
    private readonly IMessageBusDriver driver;
    private int isRunning;
    private readonly MessageBusDiagnostics diagnostics = null;

    public MessageBus(IMessageBusDriver driver, SerializerRegistry serializerRegistry, ITypeIdentifier typeIdentifier, ILoggerFactory loggerFactory, bool isDebug = false)
    {
        this.driver = driver;
        this.publisher = new MessageBusPublisher(serializerRegistry, driver, typeIdentifier);
        this.receiver = new MessageBusReceiver(serializerRegistry, typeIdentifier, loggerFactory.CreateLogger<MessageBusReceiver>());
        if (isDebug)
            diagnostics = new MessageBusDiagnostics(driver, loggerFactory.CreateLogger<MessageBusDiagnostics>());
    }

    void AssertIsRunning()
    {
        if (this.isRunning == 0)
            throw new InvalidOperationException($"{nameof(StartAsync)} must be called before calling other methods.");
    }

    public async Task SubscribeAsync(string topic, QualityOfService qos, CancellationToken cancellationToken = default)
    {
        AssertIsRunning();
        await this.driver.SubscribeAsync(topic, qos, null, null, null, cancellationToken);
    }

    public async Task UnsubscribeAsync(string topic, CancellationToken cancellationToken = default)
    {
        AssertIsRunning();
        await this.driver.UnsubscribeAsync(topic, cancellationToken);
    }

    public async Task StartAsync(Func<object, Task> receiveApplicationMessage)
    {
        if (Interlocked.CompareExchange(ref this.isRunning, 1, 0) == 1)
            throw new InvalidOperationException("Already running.");

        this.receiver.OnMessageDecoded = receiveApplicationMessage;
        await this.driver.StartAsync(this.receiver.ReceiveMessageFromMessageBusDriver);
    }

    public async Task StopAsync()
    {
        if (Interlocked.CompareExchange(ref this.isRunning, 0, 1) == 0)
            throw new InvalidOperationException("Not running.");

        await this.driver.StopAsync();
    }

    public async Task PublishAsync(object notification, string busPath, QualityOfService qos = QualityOfService.AtMostOnce, CancellationToken cancellationToken = default)
    {
        AssertIsRunning();
        await this.publisher.PublishAsync(notification, busPath, qos, cancellationToken);
    }

    public async Task PublishAsync(object notification, Dictionary<string, string[]> pathTokens = null, QualityOfService qos = QualityOfService.AtMostOnce, CancellationToken cancellationToken = default)
    {
        AssertIsRunning();
        await this.publisher.PublishAsync(notification, pathTokens, qos, cancellationToken);
    }

    public void Dispose()
    {
        this.driver.Dispose();
        this.diagnostics?.Dispose();
    }
}

I think what i was missing was a "builder" class that could take care of the construction, and allow the interfaces to be returned.
Not all transport types needed to be "started", so i left that concern to the builder of the specific bus (i.e. MQTT transport would have different requirements to Kafka for instance).
Any "stopping" can be done on dispose - any one not wanting to receive messages before then can unsubscribe via the ISubscriber interface.
So we build the base MessageBusBuilderBase class:
public abstract class MessageBusBuilderBase
{
    private MessageBusDiagnostics messageBusDiagnostics;
    public bool IsDebugEnabled => messageBusDiagnostics != null;

    protected abstract Task<ITransport> NewTransportAsync(ILoggerFactory loggerFactory, Func<TransportMessage, Task> receiveMessage);

    public async Task<BuiltInterfaces> CreateAsync(IMessageSerializer[] serializers, ITypeIdentifier typeIdentifier, ILoggerFactory loggerFactory, bool isDebug = false)
    {
        var serializerRegistry = new SerializerRegistry(serializers);
        var receiver = new MessageDeserializer(serializerRegistry, typeIdentifier, loggerFactory.CreateLogger<MessageDeserializer>());
        var transport = await NewTransportAsync(loggerFactory, receiver.ReceiveMessageFromTransport);
        var publisher = new MessagePublisher(serializerRegistry, transport, typeIdentifier);
        var subscriber = new MessageSubscriber(transport);

        // chain up event handler functions
        receiver.OnMessageDecoded = subscriber.ReceiveMessageFromDeserializer;

        if (isDebug)
            this.messageBusDiagnostics = new MessageBusDiagnostics(transport, loggerFactory.CreateLogger<MessageBusDiagnostics>());

        return new BuiltInterfaces(publisher, subscriber);
    }
}

And the specific derived class for MqttMessageBusBuilder:
public class MqttMessageBusBuilder : MessageBusBuilderBase
{
    private readonly MqttSettings settings;
    public MqttMessageBusBuilder(MqttSettings settings)
    {
        this.settings = settings;
    }

    protected override async Task<ITransport> NewTransportAsync(ILoggerFactory loggerFactory, Func<TransportMessage, Task> receiveMessage)
    {
        var logger = loggerFactory.CreateLogger<MqttMessageBusBuilder>();
        logger.LogDebug($"{nameof(MqttMessageBusBuilder)} is creating a new {nameof(MqttConnection)} and will connect to {this.settings.Server}:{this.settings.Port}.");

        var connection = new MqttConnection(this.settings, receiveMessage, loggerFactory.CreateLogger<MqttConnection>());

        await connection.StartAsync();
        logger.LogDebug($"{nameof(MqttConnection)} has been started via {nameof(MqttConnection.StartAsync)}");

        return connection;
    }
}

With the above, we now have the interfaces chained correctly, and can register the instances with a inversion of control container clearly separating the concerns of publishing and subscribing:
var mqttBusBuilder = new MqttMessageBusBuilder(settings);
var serializers = new [] { MessagePackSerializer };
var idStrategy = new IdentifyUsingTransportHeaders();
mqttBusBuilder
    .Create(serializers, idStrategy, loggerFactory, var out publisher, var out subscriber);

container.RegisterSingleton(publisher);
container.RegisterSingleton(subscriber);

With regards to "stopping" the bus, I could create a BusManager class to manage a list of created busses and call StopAsync on call - alternatively will just perform this transparently by stopping in Dispose() and let that be a concern of the bus itself as i see other busses do not have Start or Stop functions so I shouldn't let that be the driver of my interface design.


Improve Trie manipulation in Haskell code I have the following data structure:

data XComposeString = XComposeString { keyEvents :: [Event], s :: Result }
  deriving (Eq, Show)

data XComposeFile = XComposeFile { strings :: [XComposeString] }
  deriving (Eq, Show)

Where Event and Result just String or some combination of several String.

I'm creating a Trie from list of type [(keyEvents x, [s x])]:

constructTrie list = M.fromListWith comp list
  where
    comp = \old new -> old ++ new

I want to check on duplicates and prefix overlaps.

duplicates :: M.TrieMap Map.Map Event [Result] -> String
duplicates m = M.showTrie list ""
  where
    list = M.filter (\v -> length v /= 1) m

prefixOverlap :: M.TrieMap Map.Map Event [Result] -> String
prefixOverlap m = M.showTrie list ""
  where
    list = M.filterWithKey (\k v -> not $ null $ catMaybes [M.lookup x m | x <- reverse (inits (init k))]) m

How can I improve this, so, there would be three functions: one to create, one to check on duplicates and one to check on prefix overlaps.

Useful imports and types definition:

import Data.List(inits)
import Data.Maybe(catMaybes)
import qualified Data.Map as Map
import qualified Data.ListTrie.Map as M

type Event = String
type Result = String

Also, cabal install list-tries is needed for ListTrie data structure.

Can you add minimal imports so your code compiles? I had to guess:

import Data.ListTrie.Map as M hiding (null)
import qualified Data.Map as Map
import Data.Maybe
import Data.List

data Event = Event deriving (Show, Eq, Ord)
data Result = Result deriving (Show, Eq)

constructTrie can be easily improved by inlining comp:

constructTrie list = M.fromListWith (++) list

Or even to constructTrie = M.fromListWith (++) but that requires turning off monomorphism restriction or adding an explicit signature for constructTrie, so it's up to your taste.

Also you can deduplicate showTrie calls and long trie signatures:

mshow list = M.showTrie list ""

type ResultTrie = M.TrieMap Map.Map Event [Result]

duplicates :: ResultTrie -> String
duplicates m = mshow list
  where
    list = M.filter (\v -> length v /= 1) m

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow list
  where
    list = M.filterWithKey (\k v -> not $ null $ catMaybes [M.lookup x m | x <- reverse (inits (init k))]) m

Now it's beneficial to inline list variables in both functions:

duplicates :: ResultTrie -> String
duplicates m = mshow $ M.filter (\v -> length v /= 1) m

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow $ M.filterWithKey (\k v -> not $ null $ catMaybes [M.lookup x m | x <- reverse (inits (init k))]) m

Now you can get rid of m in duplicates:

duplicates :: ResultTrie -> String
duplicates = mshow . M.filter (\v -> length v /= 1)

In prefixOverlap the line is too long, I can split it. I chose to extract the (\ -> ) construct. Note that m is in more that 1 place so we cannot use ..

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow $ M.filterWithKey f m where
    f k _ = not $ null $ catMaybes [M.lookup x m | x <- reverse (inits (init k))]

Now remove extra parentheses in the comprehension:

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow $ M.filterWithKey f m where
    f k _ = not $ null $ catMaybes [M.lookup x m | x <- reverse $ inits $ init k]

From here I'll only work on f.

Now a trickier part. Long chains of $ are equivalent to . following one $ in the end:

    f k _ = (not . null . catMaybes) [M.lookup x m | x <- reverse $ inits $ init k]

not . null . catMaybes is a function of [Maybe a] -> Bool that returns True only if there is any Just in the list. I can literally translate this sentence into Haskell: it's just any isJust:

    f k _ = any isJust [M.lookup x m | x <- reverse $ inits $ init k]

Now even trickier: your comprehension is just a map:

    f k _ = any isJust $ map (\x -> M.lookup x m) $ reverse $ inits $ init k

Congratulations, we can again turn a chain of $ into a chain of .:

    f k _ = any isJust . map (\x -> M.lookup x m) . reverse . inits . init $ k

Now remember that . is associative so you can put parentheses in the middle of the chain wherever you want:

any isJust . (map (\x -> M.lookup x m) . reverse) . inits . init

Now we have map something . reverse. And it doesn't matter when you reverse - before or after map:

map something f . reverse = reverse . map something

That is, map and reverse commute and we can swap them and remove parens:

any isJust . reverse . map (\x -> M.lookup x m) . inits . init

Now we can put around any and reverse. It's clear that any works the same way for straight and reversed lists, that is, any something . reverse = any something, so we can throw reverse away:

any isJust . map (\x -> M.lookup x m) . inits . init

And put back into prefixOverlap:

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow $ M.filterWithKey f m where
    f k _ = any isJust . map (\x -> M.lookup x m) . inits . init $ k

Here is the full version of the code:

import Data.ListTrie.Map as M hiding (null, map)
import qualified Data.Map as Map
import Data.Maybe
import Data.List

type Event = Char
type Result = ()
type ResultTrie = M.TrieMap Map.Map Event [Result]

data XComposeString = XComposeString { keyEvents :: [Event], s :: Result }
  deriving (Eq, Show)

data XComposeFile = XComposeFile { strings :: [XComposeString] }
  deriving (Eq, Show)

constructTrie list = M.fromListWith (++) list

mshow :: ResultTrie -> String
mshow list = M.showTrie list ""

duplicates :: ResultTrie -> String
duplicates = mshow . M.filter (\v -> length v /= 1)

prefixOverlap :: ResultTrie -> String
prefixOverlap m = mshow $ M.filterWithKey f m where
    f k _ = any isJust . map (\x -> M.lookup x m) . inits . init $ k

Note that I used fake Event and Result types. I also checked that old and new prefixOverlap are the same using QuickCheck and smallcheck libraries:

prefixOverlapOld :: ResultTrie -> String
prefixOverlapOld m = M.showTrie list ""
  where
    list = M.filterWithKey (\k v -> not $ null $ catMaybes [M.lookup x m | x <- reverse (inits (init k))]) m

prop_foo x = prefixOverlap m == prefixOverlapOld m where
    m = M.fromList $ map (first (++ "X")) x

first is from Control.Arrow.

The prefixOverlap probably can still be improved with using lookupPrefix and/or children functions from Data.ListTrie.Map.

Here is my take:

prefixOverlapNew :: ResultTrie -> String
prefixOverlapNew = mshow . M.unions . map (\(p,m) -> M.addPrefix [p] m) . Prelude.filter (\(p, m) -> M.size m /= 1 || (null $ fst $ head $ M.toList m)) . Map.toList . M.children1

Note that it differs from both original prefixOverlapOld and refactored prefixOverlap:

QuickCheck found that they differ on a map with 2 keys "fo" and "f". The prefixOverlapNew  version correctly shows both "fo" and "f" as they overlap. The original version(s) show only value on "fo" key which is dubuios.

Here is an improved version using list comprehension to combine filter and map:

prefixOverlapNew :: ResultTrie -> String
prefixOvelapNew mm = mshow $
    M.unions [M.addPrefix [p] m | (p, m) <- Map.toList $ M.children1 mm,  M.size m /= 1 || null (fst $ head $ M.toList m)]

Moved f out:

prefixOverlapNew mm = mshow $ M.unions [M.addPrefix [p] m | (p, m) <- Map.toList $ M.children1 mm, f m] where
        f m =  M.size m /= 1 || null (fst $ head $ M.toList m)

Finally, a version in list monad, just for fun:

prefixOverlapNew :: ResultTrie -> String
prefixOverlapNew mm = mshow $ M.unions $ do
        (p, m) <- Map.toList $ M.children1 mm
        guard $ M.size m /= 1 || null (fst $ head $ M.toList m)
        return $ M.addPrefix [p] m

guard is from Control.Monad.


Compare multiple values of an array in the same time in Javascript As of now the below code working As expected. But in future if the newFilterArr have new record again I want to reedit this functionality. So how to overcome this?

function Optimiz() {
    let courseFilter = this.newFilterArr.find(k => k.key === 'courseId'),
        rateTypeFilter = this.newFilterArr.find(k => k.key === 'rateTypeId');

    this.rateSetupDataUI = this.originalRateSetupDataUI.filter(x => {
        if (courseFilter && rateTypeFilter) {
            return x['courseId'] === courseFilter.filtered.id && x['rateTypeId'] === rateTypeFilter.filtered.id;
        } else if (courseFilter && !rateTypeFilter) {
            return x['courseId'] === courseFilter.filtered.id
        } else if (!courseFilter && rateTypeFilter) {
            return x['rateTypeId'] === rateTypeFilter.filtered.id;
        }
    });
}

let courseFilter = this.newFilterArr.find(k => k.key === 'courseId'),
    rateTypeFilter = this.newFilterArr.find(k => k.key === 'rateTypeId');

I would consider making your filter an object instead of an array. The keys are probably unique and known ahead of time, thus could be a type.

    if (courseFilter && rateTypeFilter) {
        return x['courseId'] === courseFilter.filtered.id && x['rateTypeId'] === rateTypeFilter.filtered.id;
    } else if (courseFilter && !rateTypeFilter) {
        return x['courseId'] === courseFilter.filtered.id
    } else if (!courseFilter && rateTypeFilter) {
        return x['rateTypeId'] === rateTypeFilter.filtered.id;
    }

This code is a good example of a function doing too many things. You can just progressively filter, filtering by courseId first, and whatever results from that, you filter with rateTypeId.  The condition with the && is not necessary.

This could be rewritten as:

class Filter {
  courseId:string
  rateTypeId:string
}

function Optimiz() {
  const filter:Filter = this.newFilterObj

  // Match only if filter is a truthy value. Otherwise, just add it.
  this.rateSetupDataUi = this.originalRateSetupDataUI
    .filter(x => filter.courseId ? x.courseId === filter.courseId : true)
    .filter(x => filter.rateTypeId ? x.rateTypeId === filter.rateTypeId : true)
}

Nitpicks

I advise against a single let/var/const and recommend a let/var/const per variable. Prepending/appending requires needlessly modifying the first and last lines, respectively. And if you move variables around, you will still have to add let/var/const.
x['courseId'] and x['rateTypeId'] don't need to be in bracket notation, as the property names don't have invalid characters.


Class design question I have Winform application and I want to separate the button click event from the main form.

My application has Add button which adds files into my Listbox and until now all my code was inside this button click event and caused my code to look complicated.
So I created this class:

public class ListboxFile
{
    public delegate void OnFileAdd(string file);
    public event OnFileAdd OnFileAddEvent;
    private static List<string> _files;

    public ListboxFile()
    {
        _files = new List<string>();
    }

    public void add(string file)
    {
        _files.Add(file);
        OnFileAddEvent(file);
    }

    public void remove(string file)
    {
        if (_files.Contains(file))
        {
            _files.Remove(file);
        }
    }

    public void clear()
    {
        _files.Clear();
    }

    public List<string> list
    {
        get { return _files; }
    }
}

in the constructor I am initiate my List and each file that comes and add to my List raise an event and from the main form this file also add to my Listbox

this is the event from the main form that responsible to add the file into the Listbox:

private void lbf_OnFileAddEvent(string file)
{
    if (InvokeRequired)
    {
        this.Invoke((MethodInvoker)delegate
        {
            listBoxFiles.Items.Add(file);
        });
    }
    else
    {
        listBoxFiles.Items.Add(file);
    }
}

and this is the Add button click that takes the files that the user chooses to add:

    private void btnAddfiles_Click(object sender, EventArgs e)
    {
        string fileToAdd = string.Empty;
        System.IO.Stream stream;
        OpenFileDialog thisDialog = new OpenFileDialog();
        thisDialog.InitialDirectory = (lastPath.Length > 0 ? lastPath : "c:\\");
        thisDialog.Filter = "(*.snoop, *.pcap, *.cap, *.net, *.pcapng, *.5vw, *.bfr, *.erf, *.tr1)" +
            "|*.snoop; *.pcap; *.cap; *.net; *.pcapng; *.5vw; *.bfr; *.erf; *.tr1|" + "All files (*.*)|*.*";
        thisDialog.FilterIndex = 1;
        thisDialog.RestoreDirectory = false;
        thisDialog.Multiselect = true;
        thisDialog.Title = "Please Select Source File";

        if (thisDialog.ShowDialog() == DialogResult.OK)
        {
            if (thisDialog.FileNames.Length > 0)
            {
                lastPath = Path.GetDirectoryName(thisDialog.FileNames[0]);
            }

            ListboxFile lbf = new ListboxFile();
            lbf.OnFileAddEvent += lbf_OnFileAddEvent;
            BackgroundWorker backgroundWorker = new BackgroundWorker();
            backgroundWorker.WorkerReportsProgress = true;
            backgroundWorker.DoWork +=
            (s3, e3) =>
            {
                foreach (String file in thisDialog.FileNames)
                {
                    try
                    {
                        if ((stream = thisDialog.OpenFile()) != null)
                        {
                            int numberOfFiles = thisDialog.SafeFileNames.Length;
                            using (stream)
                            {
                                lbf.add(file); //add the file
                                lastPath = Path.GetDirectoryName(thisDialog.FileNames[0]);
                            }
                        }
                    }

                    catch (Exception ex)
                    {
                        MessageBox.Show("Error: Could not read file from disk. Original error: " + ex.Message);
                    }
                }
            };

            backgroundWorker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(
                (s3, e3) =>
                {
                    updateUi("bla bla"); //update the gui
                });

            backgroundWorker.RunWorkerAsync();
        }
    }

Would love to hear comments about this design since I am new developer.

My first reaction is that your click handler is doing way too much work on its own.  You should break up the code into smaller methods to make it easier to maintain.

Move all the OpenFileDialog setup to the designer file.  This will make it a field rather than local variable, preventing unnecessary object creation when a user clicks Add multiple times.  Additionally, it will wire-up the proper Dispose call for you.
Take the code for opening the files and put it into a separate method.
Split off your worker event handlers into class methods rather than anonymous methods, better separating out method responsibilities.

Other things I would do:

Pass the filenames to the BackgroundWorker in its RunWorkerAsync(Object) overload.  This prevents synchronization issues if a user clicks add while the worker is still running.
Folowing the Single Responsibility Principle, you should move the file testing into the ListBoxFile class or some other helper class.  UI views should not typically handle I/O.
Make ListBoxFiles._files non-static.  If you want singleton behavior for some reason, you can keep a static reference to a ListBoxFiles object.  Providing instance-based methods but backing them with a static collection could be a bit jarring to callers, failing the Principle of Least Surprise.
Instead of using Stream, it may be more clear to use File.OpenRead(string).
With either Stream or File.OpenRead, you need to dispose the streams
Instead of raising an add event on ListBoxFile, you should instead data bind your list control to a BindingList<T>.  That will handle automatically updating your control as items are added/removed.