How can I optimize my Von Neumann neighborhood algorithm? I am working on a small project that requires finding Von Neumann neighborhoods in a matrix. Basically, whenever there is a positive value in the array, I want to get the neighborhood for that value. I used an iterative approach because I don't think there's a way to do this in less than O(n) time as you have to check every single index to make sure all of the positive values are found. My solution is this:
is_counted = {}
recursions = []

def populate_dict(grid, n):
    rows = len(grid)
    cols = len(grid[0])
    for i in range(rows):
        for j in range(cols):
            if grid[i][j] > 0:
                find_von_neumann(grid, i, j, n, rows, cols)

    return len(is_counted.keys())

def find_von_neumann(grid, curr_i, curr_j, n, rows, cols):
    #print(curr_i, curr_j)
    recursions.append(1)
    if n == 0:

        cell = grid[curr_i][curr_j]

        if cell > 0:
            key = str(curr_i) + str(curr_j)
            if key not in is_counted:
                is_counted[key] = 1

    if n >= 1:

        coord_list = []
        # Use Min so that if N > col/row, those values outside of the grid will not be computed.
        for i in range(curr_i + min((-n), rows), curr_i + min((n + 1), rows)):
            for j in range(curr_j + min((-n), cols), min(curr_j + (n + 1), cols)):
                dist = abs((curr_i - i)) + abs((curr_j - j))

                if n >= dist >= -n and i >= 0 and j >= 0 and i < rows and j < cols:

                    coord_list.append([i, j])

        for coord in coord_list:
            key = str(coord[0]) + str(coord[1])
            if key not in is_counted:
                is_counted[key] = 1

neighbors = populate_dict([

    [-1, -1, -1, -1, -1, 1, 1, 1, 1, 1, 1],
    [-1, -1, -1, 1, 1, 1, 1, 1, 1, 1, 1],
    [-1, -1, -1, 1, 1, 1, 1, 1, 1, 1, 1],
    [-1, -1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [-1, -1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, -1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
    ],
    2)

print(neighbors)

I went with a hash table as my data structure but I believe that currently the worst case runtime would look very similar to O(n^2). For example if N (the distance factor) is equal to the amount of rows or columns AND every single value is positive, its going to be inefficient. Am I missing something with my solution, for example could using dynamic programming possibly help cut down on the runtime or is this already a good solution?
I'm fairly new to algorithms and runtime analysis so please correct me if I messed up on anything in my own analysis of this , thanks!

Set vs. dict
So far as I can tell, you aren't really using the value in is_counted, so it is better represented as a set than a dictionary. Also, it shouldn't be global - this prevents re-entrance and hampers testing; instead, make it a return value of find_von_neumann, merged in turn into another set in populate_dict that is returned.
Iteration
This is both very dense and re-executes a number of things that can be pre-computed:
    for i in range(curr_i + min((-n), rows), curr_i + min((n + 1), rows)):
        for j in range(curr_j + min((-n), cols), min(curr_j + (n + 1), cols)):

instead consider something like
i0 = curr_i + min((-n), rows)
i1 = curr_i + min((n + 1), rows)
j0 = curr_j + min((-n), cols)
j1 = min(curr_j + (n + 1), cols)

for i in range(i0, i1):
    for j in range(j0, j1):

Combined inequalities
You did the right thing here:
n >= dist >= -n

though I would find it more intuitive as
-n <= dist <= n

You should do the same for i and j:
0 <= i < rows and 0 <= j < cols


Optimize TI-BASIC Prime Numbers Finder I wrote a program to find all the prime numbers up to N on my TI-84, however, it is quite slow for any number above, like, 50. Are there any ways to optimize my code?
(I added comments using //)
Input N
{2}→L₂
For(I,3,N,2) // Loop through every odd number up to N starting at 3
1→J
0→H
√(I)→S
While L₂(J)≤S and H=0 // checks every previously found prime # less than sqrt(I)
If remainder(I,L₂(J))=0
1→H // used to break the loop

J+1→J
End
If H=0 // if no numbers divided evenly, add it to the list
augment(L₂, {I})→L₂

End

Disp L₂

Thanks!

For factorization, the state of the art (as of like 1999, but also today as far as I know) is something like Rob Gaebler's ABIGSIV: https://www.ticalc.org/archives/files/fileinfo/79/7923.html
The essential idea is to use the "wheel" method. Make a list of all the prime numbers between 1 and 2*3*5*7 and stash that list in L1; then, do trial-division by L1+210*I all at once. The important thing is to maximize the amount of time you spend doing divisions and minimize the amount of time you spend doing bookkeeping.


PHP associative array with two properties I'm moving a system over to PDO and since the queries are parameterized, I need to specify both the type and value of each parameter when preparing them.

Right now I have a prepare method to create the PDOStatement object:

/**
 * Prepares a statement
 * @param string $sql The query
 * @param array $data The data to bind
 * @param array $types The types of data
 * @return \PDOStatement
 */
private function prepare($sql, array $data, array $types) {
    if (sizeof(array_diff_key($types, $data)) || sizeof(array_diff_key($data, $types))) {
        throw new \InvalidArgumentException('Keys of data and types arrays do not match.');
    }

    $stmt = $this->pdo->prepare($sql);
    foreach ($data as $key => $value) {
        $stmt->bindValue($key, $value, $types[$key]);
    }
    return $stmt;
}

Places where the code is called look like this:

$sql = "UPDATE table_name SET col1 = :param1, col2 = :param2, col3 = :param3 WHERE primary_key = :id;";

$data = array('param1' => $param1, 'param2' => $param2, 'param3' => $param3, 'id' => $id);
$types = array('param1' => \PDO::PARAM_STR, 'param2' => \PDO::PARAM_STR, 'param3' => \PDO::PARAM_INT, 'id' => \PDO::PARAM_INT);

static::$db->prepare($sql, $data, $types)->execute();

It feels a bit cumbersome to specify the data and types in two separate arrays. Is there any way I can improve this?

I could make a SQLParameter class like this:

class SQLParameter {
    public $name;
    public $value;
    public $type;

    public function __construct($name, $value, $type) {
        $this->name = $name;
        $this->value = $value;
        $this->type = $type;
    }
}

But this would make the situation even worse.

I can immediately see two more options, perhaps they're something you're after, perhaps not.
Option 1
Turn $data into a multidimensional array. You would end up with something such as:
...array('param1' => [$param1, \PDO::PARAM_STR], 'param2' => [$param2, \PDO::PARAM_STR]...

Pros

Both value data and type are paired to the key, so now you can simply refer to either with array referencing.
You can play with indentations and returns to find a format that suits you. It's easy enough to separate each key/value onto a new line.

Cons

It's more condensed, which might lead to readability issues. (Do you keep in mind the 2nd pro above though)
An IDE's autoformat might place it all on one line, depending on your settings.

Option 2
Have a type searching function to automatically determine the type and assign it. This could be a simple helper function with a few conditionals, each with one of the is_* variable functions.
You would want to call the function as a parameter of bindValue().
Pros

This would scale quite well.
It's easy to alter outputs and fine-tune.
You won't have to add the type every time you add a new key/value pair.
Can even check for things such as objects and handle those properly.

Cons

More business code and less model/data code.

I would agree that the SQLParameter is overboard. If anything, maybe one of my solutions will inspire your own.


Data Access Layer and Business Objects This is my first cut at separating the data access layer out of my very old spaghetti codebase.

I have tried to keep this as simple as possible, it is for a small project, so I am not really interested in learning Spring or Hibernate or other framework, and I don't mind writing a bit of SQL.

I basically have a 1-to-1 relation of database table to business object.

This is the Data Access Layer

package dataaccesslayer;

import dbutils.handlers.IgnoreUnderscoreBeanHandler;
import dbutils.handlers.IgnoreUnderscoreBeanListHandler;
import racemanager.businessobjects.Progression;
import org.apache.commons.dbutils.QueryRunner;
import shared.SLDbHelper;

import java.sql.SQLException;
import java.util.List;

public class DalProgression {

    private static final String SQL_SELECT_ONE =
            "   SELECT * "
                    + "   FROM progressions "
                    + "  WHERE id=? ";

    private static final String SQL_SELECT_ALL =
            "   SELECT * "
                    + "   FROM progressions "
                    + "  ORDER BY name ASC ";

    public static Progression selectOne(int id) {
        QueryRunner qr;
        Progression progression;

        try {
            qr = new QueryRunner(SLDbHelper.getInstance().getDataSource());
            progression = qr.query(SQL_SELECT_ONE, new IgnoreUnderscoreBeanHandler<Progression>(Progression.class), id);
        } catch (SQLException ex) {
            throw new RuntimeException(ex);
        }

        return progression;
    }

    public static List<Progression> selectAll() {
        QueryRunner qr;
        List<Progression> progressions;

        try {
            qr = new QueryRunner(SLDbHelper.getInstance().getDataSource());
            progressions = qr.query(SQL_SELECT_ALL, new IgnoreUnderscoreBeanListHandler<Progression>(Progression.class));
        } catch (SQLException ex) {
            throw new RuntimeException(ex);
        }

        return progressions;
    }

}

This is the related Business Object

package businessobjects;

import java.util.Date;

public class Progression {

    private int id;
    private String name;
    private Date modified;
    private Date created;

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public Date getModified() {
        return modified;
    }

    public void setModified(Date modified) {
        this.modified = modified;
    }

    public Date getCreated() {
        return created;
    }

    public void setCreated(Date created) {
        this.created = created;
    }

    @Override
    public String toString() {
        return this.getName();
    }

}

Usage example

int id = 10;
Progression p = DalProgression.selectOne(id);

List<Progressions> pList = DalProgression.selectAll();

Fire away and shoot holes in my code, so I can improve it.
One thing I am not sure about is when I do joins should I have a separate business object for the result, and as such as corresponding data access layer class to perform those queries?

How small is small?  Hibernate, JPA and so on would help you get rid of a lot of boilerplate code.

That said.

I'd change selectOne to selectById, similarly for other querying functions, which I'm sure you'll get a lot of, or in general find a good, consistent naming scheme for those functions.
Keep in mind that int has a particular size if you already have a large legacy database.
If possible remove all the statics and make DalProgression a real object (so you can set and reuse QueryRunner/SLDHelper and create better tests).
Did I mention tests. (Legacy == ) please write tests. It honestly helps so much.
In order to do that get rid of the SLDHelper singleton,
which is not a good name, since it has both an unknown acronym in it (at least to me, otherwise you'd have to explain the name) and additionally Helper, which can also mean everything (the acronym goes for DalProgression as well, less so since you've mentioned Data Access Layer; still I'd say something like ProgressionRepository, ProgressionStore or so would be better). If possible I'd just avoid such names.
Move the exception conversion into a separate class so you can reuse that, even if you have to create a wrapper for the QueryRunner.
You could also just directly return from inside the try block and initialise the variables with their values directly, i.e. try { QueryRunner qr = ...; return qr.query(...); } catch ....
I'd also create a SQLRuntimeException or so in order to catch exactly those later instead of grepping for RuntimeException.
Have a plan for transactions in your API, unless you already have, that is.
IgnoreUnderscoreBeanHandler is already generic, there are options (e.g. via reflection) to get rid of the class literal argument.
Unless you have a specific use for that particular toString method I wouldn't use that, rather have something to identify the object in a stacktrace, let's say, the id ("<Progression id=42 name=whatever>").
Thinking about it, if you have the ID and the timestamps in all business objects, create an abstract base class or an interface to capture that notion.

For the joins, I guess that unless you have extra data in join tables, creating separate business objects isn't necessary.  So I'd imagine it would be easier to understand if you just have methods List<Foo> selectFooForProgression(Progression) or Map<Foo, Progression> selectFoosForProgressions(List<Progression>) in the DalProgression class, basically where you start the join from.


Appending a codepoint to an UTF8 std::string using icu4c My code is
void utf8_append(UChar32 cp, std::string& str) {
    size_t offset = str.size();
    str.resize(offset + U8_LENGTH(cp));
    auto ptr = reinterpret_cast<uint8_t*>(&str[0]);
    U8_APPEND_UNSAFE(ptr, offset, static_cast<uint32_t>(cp));
}

This works but seems ugly. Maybe I am overlooking a simpler approach?
Relevant documentation: https://unicode-org.github.io/icu/userguide/strings/utf-8.html and https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/utf8_8h.html.

Beauty is in the eye of the beholder. I say it is perfectly valid and correct code! The only thing you might get rid of is the static_cast<uint32_t>, as an UChar32, which is an alias forint32_t, will implicitly cast to uint32_t without warnings.
You could also use append() instead of resize(), avoiding the addition, and remove the temporary ptr, to finally get:
void utf8_append(UChar32 cp, std::string& str) {
    auto offset = str.size();
    str.append(U8_LENGTH(cp), {});
    U8_APPEND_UNSAFE(reinterpret_cast<uint8_t *>(&str[0]), offset, cp);
}

If you can use C++17, str.data() is slightly nicer than &str[0] in my opinion. Or you could write &str.front().


Generic Singly Linked List in C I decided to learn generic programming in C (and take a break from templates in C++) by implementing my first simple data structure in C: the singly linked list.

What I'm looking to gain out of reviews is:

Clarity/quality
Algorithmic correctness
Code style for C
Structure and design
Any bugs or issues you can find

I'm planning to use this as a basis for implementing a hash-map in C that uses chaining.

slist.h

#ifndef C_SLIST_H
#define C_SLIST_H

#include <stdbool.h>
#include <stddef.h>

struct slist_node;
struct slist;

typedef bool (*func_cmp)(const void *a, const void *b);

struct slist *slist_init(func_cmp cmp);

void slist_insert(struct slist *, void *);

void slist_free(struct slist *);

bool slist_empty(const struct slist *);

void *slist_value(const struct slist_node *);

struct slist_node *slist_find(const struct slist *, void *);

void clear(struct slist *);

size_t slist_size(const struct slist *);

void slist_remove(struct slist *, void *);
#endif

slist.c

#include <stdlib.h>
#include "slist.h"

struct slist_node {
    struct slist_node *next;
    void *data;
};

struct slist {
    struct slist_node *head, *tail;
    func_cmp compare;
};

static struct slist_node *create_node(void *value)
{
    struct slist_node *node = calloc(1, sizeof(*node));
    if (!node) { return NULL; }
    node->data = value;
    return node;
}

static bool default_compare(const void *a, const void *b)
{
    return a == b;
}

struct slist *slist_init(func_cmp func)
{
    struct slist *l = calloc(1, sizeof(struct slist));
    l->compare = func ? func : default_compare;
    return l;
}

void slist_insert(struct slist *l, void *value)
{
    if (!l) { return; }

    if (slist_empty(l)) {
        l->head = create_node(value);
        if (!l->head) { return; }
        l->tail = l->head;
        return;
    }

    l->tail->next = create_node(value);
    if (!l->tail->next) { return; }

    if (l->tail->next) {
        l->tail = l->tail->next;
    }
}

void clear(struct slist *l)
{
    if (!l) { return; }

    struct slist_node *copy = l->head;
    while (copy) {
        struct slist_node *after = copy->next;
        free(copy);
        copy = after;
    }

    l->head = NULL;
    l->tail = NULL;
}

void slist_free(struct slist *l)
{
    clear(l);
}

bool slist_empty(const struct slist *l)
{
    return l ? l->head == NULL : true;
}

void *slist_value(const struct slist_node *n)
{
    return n ? n->data : NULL;
}

size_t slist_size(const struct slist *l)
{
    if (!l || slist_empty(l)) { return 0; }

    const struct slist_node *copy = l->head;
    size_t size = 0;
    while (copy) {
        ++size;
        copy = copy->next;
    }
    return size;
}

static struct slist_node *slist_find_ahead(const struct slist *l, void *value)
{
    struct slist_node *copy = l->head;
    while (copy) {
        if ((l->compare)(copy->next->data, value)) {
            return copy;
        } else {
            copy = copy->next;
        }
    }
    return NULL;
}

struct slist_node *slist_find(const struct slist *l, void *value)
{
    if (!l || slist_empty(l)) { return NULL; }

    if ((l->compare)(l->head->data, value)) { return l->head; }

    const struct slist_node *found = slist_find_ahead(l, value);
    return found ? found->next : NULL;
}

void slist_remove(struct slist *l, void *value)
{
    if (!l || slist_empty(l)) { return; }

    if (l->head->data == value) {
        struct slist_node *copy = l->head;
        if (l->tail == l->head) {
            l->tail = copy->next;
        }
        l->head = copy->next;
        free(copy);
        return;
    }

    struct slist_node *ahead = slist_find_ahead(l, value);
    if (!ahead) { return; }

    if (ahead->next == l->tail) {
        l->tail = ahead;
    }
    struct slist_node *to_delete = ahead->next;
    ahead->next = ahead->next->next;
    free(to_delete);
}

[can't finish now--work in progress]

In create_node:

static struct slist_node *create_node(void *value)
{
    struct slist_node *node = calloc(1, sizeof(*node));
    if (!node) { return NULL; }
    node->data = value;
    return node;
}

...I see a few things I don't like. Personally, I'd prefer an explicit comparison to NULL rather than !node. I'd also rather avoid doubled-up return statements, so I'd probably write the code more like this:

static struct slist_node *create_node(void *value)
{
    struct slist_node *node = calloc(1, sizeof(*node));
    if (node != NULL)
        node->data = value;
    return node;
}

I'm also a bit less than enthused about using calloc here. You're assuming that the zero-bytes used to initialize the memory will turn into null pointers when the right number of them are treated as a pointer. That's pretty common, but not actually required. It also means that node->data gets initialized to zero bytes, only to be immediately overwritten with some other value. As a rule, I'd prefer to explicitly set the pointers:

static struct slist_node *create_node(void *value)
{
    struct slist_node *node = malloc(sizeof(*node));
    if (node != NULL) {
        node->next = NULL;
        node->data = value;
    }
    return node;
}

I'd at least consider keeping track of the current list size in the slist structure. When you insert a node, increment it. When you remove a node, decrement it. This lets you get the size for essentially zero cost with extremely minimal overhead.

I'd write slist_find_ahead using a for loop, something like:

struct slist_node *p;
for (p=list->head; p!=NULL; p=p->next)
    if (list->compare(p->data, value))
        return p;
return NULL;

As it is, slist_find_ahead will attempt to dereference copy->next->data, even when copy->next == NULL.


Non-nullable reference type default parameters in C# 8 I was working to upgrade some code to C# 8.0 and came across an interesting pattern with default parameters and non-nullable reference types. I'm working if it's clear enough to the caller.

Given this pre C# 8.0 code,

Task FooAsync(
   Bar bar = null,
   CancellationToken ct = default)
{
    bar = bar ?? new Bar(); // some sensible default
}

the bar parameter is flagged as one to potentially make nullable (Bar? bar = null). This would indicate to the caller that they can pass null.

However, if I want to encourage the caller not to pass null for the parameter, I could do something like this in C# 8.0:

Task FooAsync(
   Bar bar = null!,
   CancellationToken ct = default)
{
    bar ?? = new Bar(); // some sensible default
}

Is this a sensible approach to avoid passing null into the parameter? I know the caller can still do: FooAsync(bar: null!) but that obviously sticks out.

My take on this is that the function signature should not lie.
So if it can handle a null passed in, as it can, it should use

Bar? bar = null

Another approach could be to overload the function with another which only takes a single parameter. Something like ...

Task FooAsync(
   Bar bar,
   CancellationToken ct = default)
{
    ...
}

Task FooAsync(
   CancellationToken ct = default)
{
    return FooAsync(new Bar(), ct);
}


Dividing a long (arbitrary-precision) number by an integer As part of a (slightly) larger program, I needed to write a function that could take an arbitrarily large number and divide it by a small number. In this program, I will be dividing this number specifically by 2 or 5, but from my (rudimentary) testing, my function seems to work for larger divisors as well. All of the inputs in this function will be valid (they are checked before passed to this function in a different part of the program). A valid input is a non-negative, properly-represented number, things like: "123\0" but not "-1000\0" or "00100\0". So input validation is not necessarily to that extent.

#include <stdlib.h>
#include <string.h>

static inline int toNumber(char c) {
    return c - '0';
}

static inline char toCharacter(int n) {
    return n + '0';
}

char *divide(char *a, unsigned int divisor) {
    int length = strlen(a);
    char *quotient = malloc(length + 1);
    if (quotient == NULL) {
        return NULL;
    }
    int i, j;
    int localDivident = 0;
    for (i = 0, j = 0; i < length; i++) {
        if (a[i] == '0' && localDivident == 0) {
            quotient[j++] = '0';
        } else {
            if (localDivident != 0) {
                localDivident = localDivident * 10 + toNumber(a[i]);
            } else {
                localDivident = toNumber(a[i]);
            }
            int localQuotient = localDivident / divisor;
            if (localQuotient > 0) {
                quotient[j++] = toCharacter(localQuotient);
                localDivident -= localQuotient * divisor;
            }
        }
    }
    quotient[j++] = '\0';
    char *resized = realloc(quotient, j);
    return (resized == NULL) ? quotient : resized;
}

I have tested a variety of inputs and I have found that this function works surprisingly well, especially since I initially designed it with the intention of only handling single-digit divisors. However, I have found that it returns "\0" when the divisor is greater than the numeric interpretation of the input string. For example, divide("8\0", 9) returns "\0". One possible solution is adding a basic check like:

if (strtol(a, NULL, 10) < divisor) {
    char *trivialQuotient = malloc(2);
    if (trivialQuotient == NULL) {
        return NULL;
    }
    trivialQuotient[0] = '0';
    trivialQuotient[1] = '\0';
    return trivialQuotient;
}

But I find this solution inelegant, especially since the added computation of strtol will be pointless for the vast majority of executions, and I feel that there is a more elegant way for my algorithm to account for this scenario. I know that the line:

if (localQuotient > 0) {

is responsible for the result, but I could not think of a good way to modify it.

Also, I am torn between replacing the use of the static inlined functions like toNumber with a simple macro, like:

#define CTON(X) ((X) - '0')

I have read that the compiler may not always inline functions that are attributed with the inline modifier, and I know that the function should be inlined, which is why I am considering just using the macro.

(Also, I am aware that this function returns memory allocated using malloc which means that callers need to eventually free it.)

Bug
Given the input 91 and dividing by 9, your program gives 1 as the answer instead of 10.  The problem is here:
        int localQuotient = localDivident / divisor;
        if (localQuotient > 0) {
            quotient[j++] = toCharacter(localQuotient);
            localDivident -= localQuotient * divisor;
        }

You have an if statement that excludes localQuotient == 0, but it shouldn't.  This is what prevents the 0 from being output.
Code simplification
You also have two other special cases dealing with zeros that can be removed, because they are properly handled by the normal case code.  Your main loop should be reduced from 16 lines to these 6 lines:
for (i = 0, j = 0; i < length; i++) {
    localDivident = localDivident * 10 + toNumber(a[i]);
    int localQuotient = localDivident / divisor;
    quotient[j++] = toCharacter(localQuotient);
    localDivident -= localQuotient * divisor;
}


BestApproximationDiv2 problem in Haskell The task is to write a function findFraction maxDen number to find a short but good rational approximation to a decimal representation of a number — problem statement on TopCoder:

  Given a fraction F = A/B, where 0 <= A < B, its quality of approximation with respect to number is calculated as follows:

  Let S be the decimal fraction (infinite or finite) representation of F.
  Let N be the number of digits after the decimal point in number. If number has trailing zeros, all of them are considered to be significant and are counted towards N.
  If S is infinite or the number of digits after the decimal point in S is greater than N, only consider the first N decimals after the decimal point in S. Truncate the rest of the digits without performing any kind of rounding.
  If the number of digits after the decimal point in S is less than N, append trailing zeroes to the right side until there are exactly N digits after the decimal point.
  The quality of approximation is the number of digits in the longest common prefix of S and number. The longest common prefix of two numbers is the longest string which is a prefix of the decimal representations of both numbers with no extra leading zeroes. For example, "3.14" is the longest common prefix of 3.1428 and 3.1415.

  […] You are only allowed to use fractions where the denominator is less than or equal to maxDen. Find an approximation F = A/B of number such that 1 <= B <= maxDen, 0 <= A < B, and the quality of approximation is maximized. Return a String formatted "A/B has X exact digits" (quotes for clarity) where A/B is the approximation you have found and X is its quality. If there are several such approximations, choose the one with the smallest denominator among all of them. If there is still a tie, choose the one among those with the smallest numerator.

import Data.Char
import Data.List
import Data.Maybe

showResult :: (Int, Int, Int) -> String
showResult (a, b, x) = show a ++ "/" ++ show b ++ " has "
                    ++ show x ++ " exact digits"

compareDigits :: [Int] -> [Int] -> Ordering
compareDigits xs [] = EQ
compareDigits (x:xs) (y:ys) = case compare x y of
                                  EQ -> compareDigits xs ys
                                  order -> order

toDigit :: Char -> Int
toDigit c = ord c - ord '0'

preciseDivision :: Int -> Int -> [Int]
preciseDivision a b = div a' b : preciseDivision (mod a' b) b
           where a' = 10 * a

estimate :: [Int] -> Double
estimate = sum . zipWith (flip (/)) (iterate (*10) 10) .
           map fromIntegral

bestNumerator :: [Int] -> Double -> Int -> Maybe Int
bestNumerator ds p b =
    case find atLeast . map (pair ds b) $ [pivot .. ] of
        Just (a, EQ) -> Just a
        otherwise -> Nothing
    where pivot = truncate $ p * fromIntegral b
          pair ds b a = (a, compareDigits (preciseDivision a b) ds)
          atLeast (a, order) = order /= LT

bestResult :: Int -> [Int] -> Maybe (Int, Int, Int)
bestResult n ds = fromMaybe Nothing . find isJust .
                  map (toResult ds $ estimate ds) $ [1..n]
    where toResult ds p b = case bestNumerator ds p b of
                                Just a -> Just (a, b, 1 + length ds)
                                Nothing -> Nothing

findFraction' :: Int -> [Int] -> (Int, Int, Int)
findFraction' n = fromJust . last . takeWhile isJust .
                  map (bestResult n) . inits

findFraction :: Int -> String -> String
findFraction n = showResult . findFraction' n .
                 map toDigit . tail . tail

I'm most worried by bestNumerator and bestResult here. I'm using a find idiom that I invented myself, and I wonder if cleaner, more Haskelly alternatives exist.

Hmmm, not much ideas, only syntax:

showResult :: (Int, Int, Int) -> String
showResult (a, b, x) = concat [show a, "/", show b, " has ", show x, " exact digits"]

preciseDivision :: Int -> Int -> [Int]
preciseDivision a b = let (d,r) = divMod (10*a) b in d : preciseDivision r b

...where toResult ds p b = fmap (\a -> (a, b, 1 + length ds)) $ bestNumerator ds p b


Critique of realloc() wrapper Seeking code correctness and best practices.

I would like to re-allocate various amounts of memory (0 or more) such that on systems that may return NULL for an allocation of 0 does not falsely imply a memory allocation error.

Given:

void * (with a valid value or NULL already)
size_t (new desired size)

Receive:

void * (new pointer)
int (failure status)

#include <assert.h>
#include <stdlib.h>

int ReallocAndTest(char **Buf, size_t NewSize) {
  assert(Buf);
  char *NewBuf = realloc(*Buf, NewSize);
  if ((NewBuf == NULL) && (NewSize > 0)) {
    return 1;  // return failure
  }
  *Buf = NewBuf;
  return 0;
}

I came across the below references, which imply a realloc() return of NULL is not always a failure.  The lone special case occurs with size is 0.  The return value may or may not be NULL.

  C11dr 7.22.3.5 The realloc function 4 The realloc function returns a
  pointer to the new object (which may have the same value as a pointer
  to the old object), or a null pointer if the new object could not be
  allocated.

  C11dr §J.1 Unspecified behavior.  — The amount of storage allocated by
  a successful call to the calloc, malloc, or realloc function when 0
  bytes was requested (7.22.3).

  C11dr §J.3.12 Implementation-defined behavior.  Whether the calloc,
  malloc, and realloc functions return a null pointer or a pointer to an
  allocated object when the size requested is zero (7.22.3).

Discusses how a memory allocation of 0 is useful.
Answer: when a routine has a malloc(size), testing for size != 0 is not needed.
what's the point in malloc(0)?
Asks if realloc(ptr), 0) is the same as free(ptr).
Answer: Not necessarily.
What happens if I re-alloc and the new size is 0. Is this equivalent with a free?
Allocation wrapper set.
Answer: Does not address realloc(ptr, 0).
Are these memory-allocation wrapper functions kosher with all C compilers?

[Answer]

Use void* rather than char *.
With a 0 size, avoid returning the implementation defined
..a) NULL or
..b) ptr that cannot be referenced.
and return a pointer to insure only 1 of the 2 is ever returned.

On point #2, I only slightly favor returning NULL.

Standard naming convention in C is usually lower_case_with_underscore.
You should be using void * rather than char *
I would consider making the return value deterministic when the new size is 0 by always returning NULL rather than the implementation defined behaviour (which could technically be a pointer which is not NULL but still not allowed to be dereferenced). free guarantees by spec that free(NULL) is legal.


How to optimize Cookie Parsing? I have written this code for cookie parsing. Can anyone tell me what optimizations we can do in this or what can we do to make it more concise?

function parseCookies(cookies) {
  var cookie,
    cookieParts,
    parsedCookies = [];

  if (!cookies || !cookies.length) {
    return null;
  }

  // Remove domain from each cookie to decrease the size of cookie (Not needed)
  for (var i = 0; i < cookies.length; i++) {
    cookie = cookies[i];
    cookieParts = cookie.split(';');

    for (var index2 = cookieParts.length - 1; index2 >= 0; index2--) {
      var pair = cookieParts[index2],
        key = pair.split('=')[0];

      if (key && key.trim().toLowerCase() == 'domain') {
        cookieParts.splice(index2, 1);
      }
    }

    cookie = cookieParts.join(';');
    parsedCookies.push(cookie);
  }

  return parsedCookies;
}

Like I wrote in my comment, the function you've got isn't really a parser. Yes, it parses the strings you pass it, but that's not its purpose, as far as I can tell. Parsing is a byproduct of the actual purpose, which seems to be to sanitize the strings by removing the domain key/value pair.

Now, if you're just looking to remove that key/value pair, you could simply do

function sanitizeCookies(strings) {
  return strings.map(function (cookieString) {
     return cookieString.replace(/\bdomain\s*=[^;]*;?/ig, "");
  });
}

Like @tomdemuyt, I'm using Array.map() so you won't get null back if you pass it an empty array; you'll just get another empty array (and, frankly, that makes more sense, I think).


Resolving Git conflicts with Javascript Challenge:
Resolve git conflict by resolveGitConflict(linesArray, resolveMode), where
linesArray is an array of strings
Example:
[
    "context1",
    "context2",
    "<<<<<<<",
    "my changes1",
    "=======",
    "their changes1",
    ">>>>>>>",
    "context3",
    "context4",
    "<<<<<<<",
    "my changes2",
    "=======",
    "their changes2",
    ">>>>>>>",
    "context5",
    "context6"
]

resolveMode -  a string indicate:

m: Keep only current branch's changes
t: Keep only other branch's changes
tm: Keep both changes, but put other branch's changes before current branch's changes
mt - Keep both changes, but put other branch's changes after current branch's changes
none - Keep no changes

Review
We have three solutions for that challenge. Please compare  all of them and tell which one is better.
Solution 1
const resolveGitConflict = (linesArray, resolveMode, t = []) =>
  linesArray.reduce((p, c) => p.concat(
    t.length ?
      c === '>>>>>>>' ?
        (c = [].concat(resolveMode[0] ? t.splice(+(resolveMode[0] === 't'), 1)[0].concat(resolveMode[1] ? t[0] : []) : [])) &&
        (t = []) && c
      : c === '=======' ? t[1] = [] : t[+!!t[1]].push(c) && []
    : c === '<<<<<<<' ? t[0] = [] : [c]
  ), [])

Solution 2
function resolveGitConflict(linesArray, mode) {
    var [changes, curMode] = [{m: [], t: []}, ''];
    return linesArray.map(line => {
        var newLines = [];
        if (line === '<<<<<<<' || line === '=======')
            curMode = line === '<<<<<<<'? 'm' : 't';
        else if (line === '>>>>>>>') {
            newLines = Array.from(mode).map(c => changes[c] || []).flat(1);
            [changes.m, changes.t, curMode] = [[], [], ''];
        } else
            (changes[curMode] || newLines).push(line);
        return newLines;
    }).flat(1);
}

Solution 3
function resolveGitConflict(lines, mode) {
  return lines.reduce((p, c) => {
    let last = p[p.length-1]
    if(typeof last === 'object' ) {
      if(c === '>>>>>>>') last = [].concat(last[mode[0]] || [], last[mode[1]] || [])
      else if(c === '=======') last.t = []
      else last[last.t ? 't' : 'm'].push(c)
      return p.slice(0, -1).concat(last)
    } else if(c === '<<<<<<<') c = {m: []}
    return p.concat(c)
  }, [])
}

It is difficult to tell "which one is better". As Jan mentioned in a comment:

"tell which one is better" - what does "better" mean? performance? code readability? ... ?

The notion of better is subjective.
I am drawn to the first solution as it uses the fewest lines, though readability suffers because the lines are quite long and there are nested ternary operators.
I notice Solution 2 uses ES6 features like arrow functions (as all solutions do) yet it uses the var keyword. Some believe there is no use for var in modern JS. The variable newLines could be declared with const if instead of re-assigning it in the else if block the length property was set to 0 to truncate it and the mapped values were spread into a call to the push method. Using const instead of let helps avoid accidental re-assignment and other bugs.


Custom DataContract Serializer public class InfoSerializer : IInfoSerializer
{
    private static readonly ILogger _logger = LoggerFactory.Instance.GetCurrentClassLogger();

    public string Serialize(BackgroundJobInfo info)
    {
        var stringBuilder = new StringBuilder();

        using (var stringWriter = new StringWriter(stringBuilder, CultureInfo.InvariantCulture))
        {
            var writer = new XmlTextWriter(stringWriter);
            new DataContractSerializer(typeof(BackgroundJobInfo)).WriteObject(writer, info);
        }

        return stringBuilder.ToString();
    }

    public BackgroundJobInfo Deserialize(string info)
    {
        var dataContractSerializer = new DataContractSerializer(typeof(BackgroundJobInfo));
        using (var xmlTextReader = new XmlTextReader(info, XmlNodeType.Document, new XmlParserContext(null, null, null, XmlSpace.None)))
        {
            try
            {
                var result = (BackgroundJobInfo)dataContractSerializer.ReadObject(xmlTextReader);

                return result;
            }
            catch (Exception e)
            {
                // hopefully, will never happen
                _logger.ErrorException(e, "Error when deserializing a BackgroundJobInfo object from string <{0}>", info);
                return null;
            }
        }
    }
}

Is it good serializer? Maybe I can improve it or I miss something?

I have a few suggestions:

In Deserialize() you swallow all types of exception.  Catching and swallowing all exceptions is generally considered to be a bad practice.  In fact, Microsoft has a compiler "design warning" for this: CA1031: Do not catch general exception types.  Instead, you should swallow exceptions that you might reasonably expect, and rethrow the rest.  E.g. if you were deserializing from a file, then it would be reasonable to catch and handle FileNotFoundException.

Swallowing all exceptions can mask future problems in the code.  For instance, if somebody were to modify BackgroundJobInfo and add a property whose type is not supported by the data contract serializer, or change the order of pre-existing properties thereby invalidating old XML (DataContractSerializer is order sensitive) then the resulting bug might not be immediately noticed.

For more, see Exception Handling and/or Is catching general exceptions really a bad thing?.
XmlTextReader and XmlTextWriter are both deprecated as of .Net 2.0.  Microsoft suggests using XmlReader.Create() and XmlWriter.Create() instead.
Your methods to serialize and deserialize XML from a string using DataContractSerializer look handy, and could usefully be made generic for reuse later.

Thus:

public class InfoSerializer : IInfoSerializer
{
    private static readonly ILogger _logger = LoggerFactory.Instance.GetCurrentClassLogger();

    public string Serialize(BackgroundJobInfo info)
    {
        return DataContractSerializerExtensions.SerializeXml(info, null); // If you want indentation, use new XmlWriterSettings { Indent = true, IndentChars = "    " };
    }

    public BackgroundJobInfo Deserialize(string info)
    {
        try
        {
            return DataContractSerializerExtensions.DeserializeXml<BackgroundJobInfo>(info);
        }
        catch (Exception e)
        {
            // hopefully, will never happen
            _logger.ErrorException(e, "Error when deserializing a BackgroundJobInfo object from string <{0}>", info);
            throw;
        }
    }
}

public static class DataContractSerializerExtensions
{
    public static string SerializeXml(this object obj, XmlWriterSettings settings = null)
    {
        if (obj == null)
            throw new ArgumentNullException("obj");
        using (var textWriter = new StringWriter())
        {
            using (var xmlWriter = XmlWriter.Create(textWriter, settings))
            {
                var serializer = new DataContractSerializer(obj.GetType());
                serializer.WriteObject(xmlWriter, obj);
            }
            return textWriter.ToString();
        }
    }

    public static T DeserializeXml<T>(string xml)
    {
        if (xml == null)
            throw new ArgumentNullException("xml");
        using (var textReader = new StringReader(xml))
        using (var xmlReader = XmlReader.Create(textReader))
        {
            var serializer = new DataContractSerializer(typeof(T));
            return (T)serializer.ReadObject(xmlReader);
        }
    }
}

Update

If you are using DataContractSerializer and wish to suppress the serialization of specific properties, you should apply data contract attributes, specifically [DataContract] to the type and [DataMember] to each member to be serialized.  Note that data contracts are opt-in so once you start to use data contract attributes, all members of a type to be serialized must be marked as data members.  For instance:

[DataContract]
public class BackgroundJobInfo
{
    [DataMember]
    public string JobName { get; set; }

    [DataMember]
    public int JobPriority { get; set; }

    // Not marked with DataMember so not serialized.
    public string Password { get; set; }
}

See Using Data Contracts.

As for creating a single static DataContractSerializer to be shared globally, I don't recommend it since there might be issues with threading.  From the docs:

  Thread Safety

  Instances of this class are thread safe except when the instance is used with an implementation of the IDataContractSurrogate or DataContractResolver.

Unless you have determined via profiling that you have performance issues due to excessive allocations, it would appear safer to not share this class between threads.

Throwing an exception trying to serialize a property with "password" in the name is not implemented out of the box with DataContractSerializer.  You should be able to adapt the XmlWriter from Custom xmlWriter to skip a certain element?, then throw an exception rather than skipping the element.


Gather list of all IDs ordered by submission date and ID by parsing file path This code is to gather a sorted list of IDs (first by ID, then by date) recursively from a directory that submitted a particular type of file (that, as you can tell, includes the word "Magic") and then outputs the IDs paired with the modified date (the closest proxy I have for the submission date) to a CSV.  (Where I then dedupe by ID to get the earliest submission for each ID/find average number of unique IDs submitted a month over the year/ the total submitted for the year):

Get-ChildItem -Recurse -Include *Magic* |
Where-Object {$_.lastWriteTime -gt '01/01/2018' -AND $_.lastWriteTime -lt '12/31/2018'} |
Select-Object -Property Directory, lastWriteTime |
ForEach-Object -Begin {
     $holder=@{}
     $results=@()
} -Process {
    $holder.id= [regex]::match($_.Directory,'(\d{8})')
    $holder.modifiedDate = Get-Date  $_.lastWriteTime -Format 'yyyy/MM/dd'
    $results+=[pscustomobject]$holder
} -End {
    $results
    }  |
Sort-Object -Property @{Expression = "id"; Descending = $True;}, @{Expression = "modifiedDate"; Descending = $False} |
Export-csv C:\Users\REDACTED\Desktop\REDACTED.csv

This is my first attempt at writing something in powershell purely using the command prompt (which is to say, not scripting in the ISE as if it were Ruby using ifs and so forth) How could I have done this better and should I have organized the pipeline differently? I tried to avoid using variables like would have in the past and focused purely on piping the data I was interested in from beginning to end.

EDIT

Based on the comment I got I made these changes. I wasn't able to get rid of all the pipelines and I haven't been able to test if it's faster or not, but I have a feeling even this version has some garbage that could go.

$sourceFiles = Get-ChildItem -Recurse -Include *Magic*
$filteredFiles = @()
ForEach($file in $sourceFiles){
    If($file.LastWriteTime.Year -eq 2018){
        $filteredFiles+=[PSCustomObject]@{
            Agency = [regex]::match($file.Directory,'(\d{8})')
            ModifiedDate = Get-Date  $file.lastWriteTime -Format 'yyyy/MM/dd'
        }
    }
}
$filteredfiles | Sort-Object -Property @{Expression = "Agency"; Descending =
$True;}, @{Expression = "ModifiedDate"; Descending = $False} |
Export-csv C:\Users\REDACTED\Desktop\REDACTED.csv -NoTypeInformation

here is a solution that uses ...

-Filter instead of -Include
this is usually faster since the filtering is done by the provider [the filesystem in this case] instead of by the cmdlet.

[PSCustomObject] object construction
faster than Select-Object for building custom objects, since it does not require building/tearing-down a pipeline. plus the Select-Object cmdlet has a great deal of complex logic that is not needed here.

$AgencyList = foreach ($FL_Item in $FileList) loop assignment
this is VASTLY faster than $AgencyList += for large collections, but only slightly faster for small ones.

$SourceDir = $env:TEMP
$Filter = '*itunes*'
$TargetYear = 2019
$ExportFile = "$env:TEMP\jasonmadesomething_AgencyList.csv"

$GCI_Params = @{
    LiteralPath = $SourceDir
    Filter = $Filter
    File = $True
    Recurse = $True
    }
$FileList = Get-ChildItem @GCI_Params |
    Where-Object {$_.LastWriteTime.Year -eq $TargetYear}

$AgencyList = foreach ($FL_Item in $FileList)
    {
    [PSCustomObject]@{
        # i don't have any suitable directory info
        #    so i used the BaseName of the file
        Agency = $FL_Item.BaseName.Split('.')[0] -replace '^Itunes_', ''
        LastWriteTime = $FL_Item.LastWriteTime
        }
    }

# this gives a list sorted by ...
#    Agency [alfa sorted 'A' to 'Z' and/or '0' to '9']
#    LastWriteTime [oldest 1st]
$AgencyList = $AgencyList |
    Sort-Object -Property Agency, LastWriteTime

# on screen
$AgencyList

# to CSV
$AgencyList |
    Export-Csv -LiteralPath $ExportFile -NoTypeInformation

truncated on screen output ...

Agency                  LastWriteTime
------                  -------------
AlbumAutoRating_Disable 2019-01-07 12:22:00 PM
AlbumAutoRating_Disable 2019-01-14 12:20:36 PM
AlbumAutoRating_Disable 2019-01-21 12:20:41 PM
AlbumAutoRating_Disable 2019-01-28 12:22:14 PM
AlbumAutoRating_Disable 2019-02-04 12:21:55 PM
[*...snip...*]
R-PC-SC_Save            2019-01-24 12:38:09 PM
R-PC-SC_Save            2019-02-07 12:37:53 PM

truncated CSV file content ...

"Agency","LastWriteTime"
"AlbumAutoRating_Disable","2019-01-07 12:22:00 PM"
"AlbumAutoRating_Disable","2019-01-14 12:20:36 PM"
"AlbumAutoRating_Disable","2019-01-21 12:20:41 PM"
"AlbumAutoRating_Disable","2019-01-28 12:22:14 PM"
"AlbumAutoRating_Disable","2019-02-04 12:21:55 PM"
[*...snip...*]
"R-PC-SC_Save","2019-01-24 12:38:09 PM"
"R-PC-SC_Save","2019-02-07 12:37:53 PM"


Seven-segment display GUI I am building a GUI to display a digital clock face (four seven-segment displays), and I have a selector to switch between 12/24-hour formats. I have working code to show both formats, but it is messy. I am looking for a better solution for this section.

The obvious repetition is the section that displays the AM/PM indicator. The only difference is that the fill color of the AM and PM switch.

Note that the time comes in as three integers, one each for hours, minutes, and seconds. These integers are in 24-hour format.

The drawDigits method takes the value to be displayed (0-9, 10=all off), and the (x,y) location of the top-left corner. It then uses a lookup to determine which segments to "turn on."

// 24 hour mode, draw time as reported
if (toggle.isSelected()) {
  drawDigit(hour/10, 20, 100);
  drawDigit(hour%10, 110, 100);
}
// 12 hour mode, display
else {
  if (hour % 12 == 0) {
    drawDigit(1, 20, 100);
    drawDigit(2, 110, 100);
  } else {
    if ((hour % 12)/10 == 1) {
      drawDigit(1, 20, 100);
    } else {
      drawDigit(10, 20, 100);
    }
    drawDigit((hour % 12)%10, 110, 100);
  }
  // show AM/PM indicator
  if (hour >= 12) {
    fill(75);
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
    fill(255, 0, 0);
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
  } else {
    fill(255, 0, 0);
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
    fill(75);
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
  }
}
drawDigit(minute / 10, 224, 100);
drawDigit(minute % 10, 314, 100);
if (second % 2 == 0) fill(240, 0, 0);
else fill(75);
ellipse(200, 145, 10, 10);
ellipse(200, 175, 10, 10);

This code was written in Processing, so the rect, ellipse, text, and fill methods are the ones built-in to Processing.

In general, I think the program is well readable, and demonstrates the idea correctly. I have some suggestions for further improving it.

Constants

Consider the following statement:

drawDigit(hour/10, 20, 100);
drawDigit(hour%10, 110, 100);

Since the values are reused more than once, I would suggest introducing constants for them:

final int HOUR_TO_DIGIT_QUOTIENT = 10;
final int  HOURS_FIRST_X = 20;
final int  HOURS_SECOND_X = 110;
final int  HOURS_Y = 100;

Of course, this holds also for all the other numeric constant, and the strings "AM" and "PM", which are repeated more than once as well.

(Btw., I would suggest introducing these constants even if each value was used only once, because this makes the code easier to understand.)

Organising code

There are some blocks of code, which could be separated into subroutines, in order to make the main code shorter, and thus more readable. E.g.:

fill(75);
rect(40, 230, 10, 10);
text("AM", 55, 240);
fill(255, 0, 0);
rect(40, 245, 10, 10);
text("PM", 55, 255);

This part could be extracted to methods, e.g. like this:

void showAmIndicactor(boolean enabled) {
    if (enabled) {
        fill(255, 0, 0);
    }
    else {
        fill(75);
    }
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
}

void showPmIndicactor(boolean enabled) {
    if (enabled) {
        fill(255, 0, 0);
    }
    else {
        fill(75);
    }
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
}

// in the main block:
if (hour >= 12) {
   showAmIndicactor(false);
   showPmIndicactor(true);
 } else {
   showAmIndicactor(true);
   showPmIndicactor(false);
 }

N.B.: I am not familiar with Processing in detail, but since from the question I understand that 'drawDigit' is an own method (i.e. not built in to Processing), I assume, that it is possible to introduce other own methods in a similar way as in Java.

Functionality

I do not understand, how the seconds will be rendered correctly. As far as I see, this is the only part that depends on the seconds:

if (second % 2 == 0) fill(240, 0, 0);
else fill(75);

But as I understand, this code does not show any digits, just fills a space with a colour. Is this the intended behaviour? If so, then I suggest documenting why is it like that (in a comment), otherwise, I propose to extend the functionality so that seconds are rendered as well.

On a side note, I recommend to use always the block-form of if-else statement (and any other statements), even if it has just one instruction per branch. In other words, I suggest rewriting it as follows:

if (second % 2 == 0) {
     fill(240, 0, 0);
}
else {
     fill(75);
}

This prevents that you add more statements (accidentally indented at the level of if-else block), and think they will be executed within if-else.

Validation

Although not part of the original problem statement, I would recommend to extend your code with validation. I.e., make sure that hours are between 0-23 (or 1-12 in case of 12-hour format), minutes and seconds are between 0-59, and throw an exception otherwise (or print an error and exit, whichever is available in Processing).

Before adding validation, it would be also interesting to experiment how your program behaves with invalid input (e.g. negative numbers, hours consisting of three digits or four, etc.)


Rock Paper Scissors in one single method in Java - Good or bad? I'm new to programming and have created a simple Rock, Paper, Scissors game. The entire program is under a single class and the main method.  I hear that's probably not the best way to code.

How should I have written this to utilize multiple classes or to adhere to best practice?

import java.util.Random;
import java.util.Scanner;

public class RPS {
    public static void main(String args[]){

    String choice, choice1;
    int num = 0;
    Random r = new Random();
    num = r.nextInt(3);

    System.out.println("Choose: Rock, Paper or Scissors.");
    Scanner userChoice = new Scanner(System.in);
    choice1 = userChoice.next();
    choice = choice1.toLowerCase();

    switch (num){
    case 0:
        String num1 = "rock";
        System.out.println("System chose Rock");
        if (choice.matches(num1)){
            System.out.println("Its a tie!");

            }
        else if (choice.matches("paper")){
            System.out.println("You win!");

        }
        else if (choice.matches("scissors")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;
    case 1:
        String num2 = "paper";
        System.out.println("System chose Paper");
        if (choice.matches(num2)){
            System.out.println("Its a tie!");

        }
        else if (choice.matches("scissors")){
            System.out.println("You win!");

        }
        else if (choice.matches("rock")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;
    case 2:
        String num3 = "scissors";
        System.out.println("System chose Scissors");
        if (choice.matches(num3)){
            System.out.println("Its a tie!");

        }
        else if (choice.matches("rock")){
            System.out.println("You win!");

        }
        else if (choice.matches("paper")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;

    }

}
}

I would make some changes, although your code is quite simple.

First, get rid of the magic Strings for rock/paper/scissors, and create an enum.

I would possibly also move the logic of checking if it's a win/draw/loose inside that enum, so it would be much easier to add new entries (imagine you want to extend that to a Rock/Paper/Scissors/Lizard/Spock.

My main recomendation is to separate getting the input from the user, and the logic of the actual program.

If these two concepts are completely separated, you could reuse the logic to build, for instance, a web interface, or get the user input in any other way (or even get two inputs from two users, and use the same logic to decide who wins!).

Check this, for instance, for an enum representation of Rock/Paper/Scissors:

https://stackoverflow.com/a/9858163/432806

Overall I would advise you to have a structure like:

main:
    input = getUserInput
    enumValue = getEnumFromInput
    secondValue = getEnumFromComputerRoll
    testWhoWins(enumValue, secondValue)

That last line could also be something like:

    enumValue.getResult(secondValue)

If you implement the logic inside the enum.

update

There's also another big downside to that monolithic approach, that hasn't been discussed yet: testing.

There is no simple way of writing unit tests for a program written in such a fashion.

To be able to have decent unit tests, you need to break up your programming logic into small pieces that can be tested independently from each other.

Think about:

How would you test transforming a String (user input), to the correct Enum/String?
How would you test generating a value in the right range, and converting into an Enum?
How would you test the logic of, given two "moves", choosing the result?

I think this is a good way of figuring out how to break up your logic: Think about how you would write tests for it.


Divide decimal by range of fractions, and then convert the fraction that is closest to a whole number to a mixed fraction My program will divide the decimal (\$30.0575\$) by a range of fractions (\$\frac{1}{1}, \frac{1}{2}, \frac{1}{4}, \dots, \frac{1}{48})\$ leaving me with list of improper fractions that look like this:

\$\frac{1}{1} * 30.0575\$
\$\frac{1}{2} * 60.115\$
\$\frac{1}{3} * 90.1725\$
\$\dots\$
\$\frac{1}{48} * 1442.76\$

My program will then "pick out" the improper fraction that contains the decimal that is closest to a whole number. It will finally convert the improper fraction that is closest to a whole number into a mixed fraction.

How can I make my program more readable?
Are there any "shortcuts" I could implement that would reduce the lines of code used?
How can I overall optimize my code so it completes the same task in a more efficient way, while still keeping readability?

private void FractionsForm_Load(object sender, EventArgs e)
        {
            var myDecimal = 30.0575;

            var results = new List<double>();

            for (double i = 1; i < 49; i++)
            {
                var fraction = 1 / i;
                var result = myDecimal / fraction;
                results.Add(result);
                Console.WriteLine("1/{0} * {1}", i, result);
            }

            WriteLineSeparator();

            var remainders = new List<double>();

            foreach (var result in results)
            {
                var roundedResult = Math.Round(result);
                var remainder = roundedResult - result;
                remainders.Add(Math.Abs(remainder));
            }

            var mostAccurateRemainder = remainders.Min();
            var mostAccurateRemainderIndex = remainders.IndexOf(mostAccurateRemainder);
            var fractionDenominator = mostAccurateRemainderIndex + 1;

            Console.WriteLine("1/{0} * {1}", fractionDenominator, results[mostAccurateRemainderIndex]);

            WriteLineSeparator();

            var integerPart = Convert.ToInt32(myDecimal);
            var integralPart = myDecimal - Math.Truncate(myDecimal);

            Console.WriteLine("{0} {1}/{2}", integerPart, Convert.ToInt32(integralPart * fractionDenominator),
                fractionDenominator);
        }

        private static void WriteLineSeparator()
        {
            Console.WriteLine(string.Empty);
            Console.WriteLine("--------------------------------------------------");
            Console.WriteLine(string.Empty);
        }

        /*

            1/1 * 30.0575
            1/2 * 60.115
            1/3 * 90.1725
            1/4 * 120.23
            1/5 * 150.2875
            1/6 * 180.345
            1/7 * 210.4025
            1/8 * 240.46
            1/9 * 270.5175
            1/10 * 300.575
            1/11 * 330.6325
            1/12 * 360.69
            1/13 * 390.7475
            1/14 * 420.805
            1/15 * 450.8625
            1/16 * 480.92
            1/17 * 510.9775
            1/18 * 541.035
            1/19 * 571.0925
            1/20 * 601.15
            1/21 * 631.2075
            1/22 * 661.265
            1/23 * 691.3225
            1/24 * 721.38
            1/25 * 751.4375
            1/26 * 781.495
            1/27 * 811.5525
            1/28 * 841.61
            1/29 * 871.6675
            1/30 * 901.725
            1/31 * 931.7825
            1/32 * 961.84
            1/33 * 991.8975
            1/34 * 1021.955
            1/35 * 1052.0125
            1/36 * 1082.07
            1/37 * 1112.1275
            1/38 * 1142.185
            1/39 * 1172.2425
            1/40 * 1202.3
            1/41 * 1232.3575
            1/42 * 1262.415
            1/43 * 1292.4725
            1/44 * 1322.53
            1/45 * 1352.5875
            1/46 * 1382.645
            1/47 * 1412.7025
            1/48 * 1442.76

            --------------------------------------------------

            1/35 * 1052.0125

            --------------------------------------------------

            30 2/35

        */

Trust the math.

        var fraction = 1 / i;
        var result = myDecimal / fraction;

is a long way to say

        var result = myDecimal * i;

Collecting results in the list is just a waste of space. You may maintain the most accurate candidate as you do compute results:

    for (double i = 1; i < 49; i++)
    {
        var result = myDecimal * i;
        var roundedResult = Math.Round(result);
        var remainder = roundedResult - result;
        if (remainder < bestRemainder) {
            bestRemainder = remainder;
            bestDenominator = i;
        }
    }


Rot -n algorithm in Java This is a rot-n (rotate n times) encrypt/decrypt algorithm, where n can be any integer (+/-).

It's written to encrypt/decrypt all the alphabets (upper/lower case) and leaving behind all non-alphas.

rot-n where n is the key, is given in the first line of input file.

Everything is working fine.  I just need help in optimizing it.

  Challenge description

  Your submission will be tested against an input file that contains
  ASCII characters. The input file starts at the very first line with a
  N-digits positive or negative integer number that is your cipher,
  followed by a new-line character. The second line starts with the
  payload of the encrypted text until the end of file. Note that only
  words (alphanumeric sequences) are encrypted and that every other
  character (i.e. punctuation) must not be processed by your algorithm
  and must also be copied ‘as is’ to the standard output.*

import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Scanner;

public class Fightsopa {

public void process(String input, int n) {
    int abcd = n;
    if (abcd < 0) {
        abcd = (26 - ((0 - abcd) % 26));
    }
    int rot = 26 - abcd % 26;  //for decrypting
 // int rot=       abcd % 26;  //for encrypting
    char out = 0;
    for (char ch : input.toCharArray()) {
        if (ch >= 65 && ch <= 90) {
            if (ch + rot <= 90) {
                out = (char) (ch + rot);
                System.out.print(out);
            } else {
                out = (char) (ch + rot - 26);
                System.out.print(out);
            }

        } else if (ch >= 97 && ch <= 122) {
            if (ch + rot <= 122) {
                out = (char) (ch + rot);
                System.out.print(out);
            } else {
                out = (char) (ch + rot - 26);
                System.out.print(out);
            }
        } else {
            System.out.print(ch);
        }
    }
}

public void handler(File fFile) throws FileNotFoundException {
    Scanner scanner = new Scanner(new FileReader(fFile));
    int rot = 0;
    int count = 0;
    while (scanner.hasNextLine()) {
        if (count == 0) {
            rot = scanner.nextInt();
            scanner.nextLine();
        } else {
            String wordp = scanner.nextLine();
            process(wordp, rot);
            System.out.println();
        }
        count++;
    }
    scanner.close();

}

public static void main(String args[]) throws FileNotFoundException {
    if (args.length > 0) {
        File fFile = new File(args[0]); //Input file having the first line as the  key
        Fightsopa fsopa = new Fightsopa();
        fsopa.handler(fFile);
    }
}
}

First, your process method violates the Single Responsibility Principle: It calculates something, and it prints it out. Such coupling makes it inflexible. It's better to return a String, and letting the caller decide what to do with it.

Second, you're code is way too complicated, handling all the different cases which are very dependent on the ASCII layout. Imagine next you want to support digits as well - oh my...

I would suggest to do something along the lines...

public String process(String input, int n) {
    String abc = "abcdefghijklmnopqrstuvwxyz";
    String abcABC = abc + abc.toUpperCase();
    int len = abcABC.length();

    Map<Character, Character> map = new HashMap<Character, Character>();
    for (int i = 0; i < len; i++) {
        map.put(abcABC.charAt(i), abcABC.charAt((i + n + len) % len));
    }

    StringBuilder sb = new StringBuilder();
    for(int i = 0; i < input.length(); i++) {
        Character ch = map.get(input.charAt(i));
        if (ch == null) {
            throw new IllegalArgumentException("Illegal character " + input.charAt(i));
        }
        sb.append(ch);
    }
    return sb.toString();
}

Note that I need to know nothing about ASCII encoding using that approach, so supporting other chars is a no-brainer.


Making a TableViewDataSource more independent of the data it utilizes I have written an ordinary tableView to display a list of players, with the following data structure that's in place.

A Conductor (which is the user), has many
Orchestra? objects.
A Orchestra? has many Player? objects

All objects besides the Conductor are cast as Optional, meaning that a Conductor could have no Orchestra, and an Orchestra could have no player.

The Conductor is a singleton object. At anytime the currentConductor could be called (similar to how you could always call currentUser in Parse or Firebase)

Currently, I am displaying all the players for a Conductor's currentOrchestra object. My concern is that my TableViewDataSource is too tightly coupled with having to know about how my Conductor class functions, and I could like to decouple some of the logic.

For instance:

class OrchestraViewDataSource: NSObject, UITableViewDataSource {

    var isCurrentOrchestraAvaliable = false

    override init() {
        super.init()

        if let _ = User.shared.currentOrchestra {
            isCurrentOrchestraAvaliable = true
        }
    }

    func numberOfSections(in tableView: UITableView) -> Int {

        if isCurrentOrchestraAvaliable {
            return 2 //Shows Performers and Non-performers
        }

        return 0
    }

    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {

        if section == 0 {
            return User.shared.currentOrchestra?.performers.count ?? 0
        } else {
            return User.shared.currentOrchestra?.nonPerformers.count ?? 0
        }

    }

    //... Additional code omitted
}

At the moment, I call User.shared.currentOrchestra?.performers.count because the user could change the line up of the performers and non-performers. Is calling the Singleton to populate my data source ok? And if not, what are better ways to make my TableViewDataSource more independent?

Thanks!

My concern is that my TableViewDataSource is too tightly coupled with having to know about how my Conductor class functions, and I could like to decouple some of the logic.

Create a protocol which defines the data in the form that the data source needs. This will separate the requirements of the data from the implementation of the class providing the data, which would give you flexibility to change the behaviour of the Orchestra object without also having to change the data source.

The orchestra object should implement the protocol and provide the data in the required format. You can use a class extension on the orchestra or conductor to add the required code.

  Is calling the Singleton to populate my data source ok? And if not, what are better ways to make my TableViewDataSource more independent?

Using Singletons often leads to problems. It would be better to follow the dependency inversion principle and pass an instance of the conductor to the data source. From the code you have shown you only seem to be using the orchestra, and so the conductor itself is not needed.

Example

The example below shows how to:

Define a protocol.
Use a class extension to have a class conform to the protocol.
Update the data source to use the protocol instead of the concrete class.
Pass the orchestra instance into the data source.

1. Define a protocol:

protocol Performers {
    var numberOfSections: Int {
        get
    }
    func numberOfPerformers(inSection: Int) -> Int
    func performer(atIndex: Int, inSection: Int) -> Performer
}

2. Add a class extension on Orchestra to conform to the Performers protocol:

extension Orchestra: Performers {
    var numberOfSections: Int {
        return 2
    }

    func numberOfPerformers(inSection section: Int) -> Int {
        if section == 0 {
            return performers.count ?? 0
        } else {
            return nonPerformers.count ?? 0
        }
    }

    func performer(atIndex index: Int, inSection section: Int) -> Performer {
        if section == 0 {
            return performers[index]
        } else {
            return nonPerformers[index]
        }
    }
}

3. Modify the data source to use the protocol

class OrchestraViewDataSource: NSObject, UITableViewDataSource {

    var performers: Performers?

    override init(performers: Performers?) {
        super.init()
        self.performers = performers
    }

    func numberOfSections(in tableView: UITableView) -> Int {
        return performers?.numberOfSections ?? 0
    }

    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return performers?.numberOfPerformers(inSection: section) ?? 0
    }

    //... Additional code omitted
}

4. Pass the orchestra instance to the data source

let currentOrchestra = User.shared.currentOrchestra
let dataSource = OrchestraViewDataSource(performers: orchestra)


Refactoring decision tree Currently I have

var l1 = [...];
var l2 = [...];
if (l1.length > l2.length){
    var i = l2.indexOf(l1[0]);
    if (i !== -1){
        action_a(i);
    }else{
        action_c();
    }
} else if (l1.length < l2.length){
    var i = l1.indexOf(l2[0]);
    if (i !== -1){
        action_b(i);
    }else{
        action_c();
    }
} else {
    action_c();
}

Which I have could refactor into the following

function foo() {
    var l1 = [...];
    var l2 = [...];
    var i = -1;
    if (l1.length > l2.length){
        i = l2.indexOf(l1[0]);
    } else if (l1.length < l2.length){
        i = l1.indexOf(l2[0]);
    }
    if (i === -1 || l1.length == l2.length) {
        action_c();
        return;
    }
    if (l1.length > l2.length){
        action_a(i);
    } else if (l1.length < l2.length){
        action_b(i);
    }
}

It's a bit better, but I'm checking for the length of the two array twice.  It's also still not as readable as I would like it to be.

Can you review the above code and offer recommendations on how to do this better?

var l1 = [...];
var l2 = [...];
if (l1.length > l2.length){
    actOnDuplicateOfFirstElementIfPresent(l1, l2, action_a);
} else if (l1.length < l2.length){
    actOnDuplicateOfFirstElementIfPresent(l2, l1, action_b);
} else {
    action_c();
}

function actOnDuplicateOfFirstElementIfPresent(la, lb, action) {
    var i = lb.indexOf(la[0]);
    if (i !== -1){
        action(i);
    }else{
        action_c();
    }
}

This Extract Method refactoring eliminates the duplication found in the first two branches.  Eliminating duplication is both an excellent motivation, and an excellent technique, for refactoring.


Calculate balance wages In the following code I'm trying to calculate Net Wage of employees and classes Earning, Deduction and WageBalance has a composition relationship with WageInfo. And there is a class called WageManager which is having a association with WageInfo, handling all operations like BalanceWage(). DataService class encapsulates all db logic.  Please review my code and let me know of any flaws.

Is the way I've implemented the relationships correct?  Would you have a method to calculate BalanceWage in a more elegant way?

    class WageManager
    {
        WageInfo _WageInfo;
        DataService _DataService;

        public WageManager(WageInfo wageInfo, DataService dataService )
        {
            _WageInfo = wageInfo;
            _DataService = dataService;
        }

        public List<WageBalance> BalanceWages()
        {

            var earningList = _DataService.GetEarningsList(); //DataService returns a List<Earning> list
            var deductionList = _DataService.GetDeductionList(); //DataService returns a List<Deduction>
            int i = 0;

            foreach (Earning e in earningList)
            {

    // Sum all earnings...

            var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;

    // Sum all deductions...

            var totalDeductions = deductionList[i].AdvanceAmount + deductionList[i].CarriedForwardAmount + deductionList[i].DeathDonationAmount + deductionList[i].EPFAmount + deductionList[i].FineAmount + deductionList[i].InsuranceInstalmentAmount + deductionList[i].LoanInstalmentAmount + deductionList[i].MealsAmount + deductionList[i].OtherDeductionAmount + deductionList[i].UniformInstalmentAmount + deductionList[i].WelfareAmount;
            var employeeID = earningList[i].EmployeeID;
            var netEarning = totalEarnings - totalDeductions;

            WageInfo wageInfo = new WageInfo();

            wageInfo.WageBalance.EmployeeID = employeeID;
            wageInfo.WageBalance.NetWage = decimal.Truncate(netEarning / 100) * 100;
            wageInfo.WageBalance.CarriedForwardAmount = netEarning - wageInfo.WageBalance.NetWage;

            _WageInfo.WageBalanceList.Add(wageInfo.WageBalance);
            i += 1;
        }

        _DataService.BalanceWages(_WageInfo.WageBalanceList);

        return _WageInfo.WageBalanceList;
    }
}

class WageInfo
{
    public DateTime WagePeriodStartDate { get; set; }
    public DateTime WagePeriodEndDate { get; set; }
    public string Reference { get; set; }
    public Deduction Deduction = new Deduction();
    public Earning Earning = new Earning();
    public WageBalance WageBalance = new WageBalance();
    public List<WageBalance> WageBalanceList = new List<WageBalance>();

    public WageInfo()
    {
    }

}

public class WageBalance
{
    public int EmployeeID {get;set;}
    public string EmployeeName { get; set; }
    public decimal NetWage { get; set; }
    public decimal CarriedForwardAmount { get; set; }
}

Formatting

formatting inside your foreach should be indented

This:

        foreach (Earning e in earningList)
        {

// Sum all earnings...

        var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;

Should look like this

        foreach (Earning e in earningList)
        {
            // Sum all earnings...
            var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;
            ...
        }

When you are adding things together (either in concatenation or variable addition) you can multi-line the expression because C# is awesome and isn't NewLine Terminated, so this:

  var totalDeductions = deductionList[i].AdvanceAmount + deductionList[i].CarriedForwardAmount + deductionList[i].DeathDonationAmount + deductionList[i].EPFAmount + deductionList[i].FineAmount + deductionList[i].InsuranceInstalmentAmount + deductionList[i].LoanInstalmentAmount + deductionList[i].MealsAmount + deductionList[i].OtherDeductionAmount + deductionList[i].UniformInstalmentAmount + deductionList[i].WelfareAmount;

can be easier read when written like this:

  var totalDeductions = deductionList[i].AdvanceAmount +
      deductionList[i].CarriedForwardAmount +
      deductionList[i].DeathDonationAmount +
      deductionList[i].EPFAmount +
      deductionList[i].FineAmount +
      deductionList[i].InsuranceInstalmentAmount +
      deductionList[i].LoanInstalmentAmount +
      deductionList[i].MealsAmount +
      deductionList[i].OtherDeductionAmount +
      deductionList[i].UniformInstalmentAmount +
      deductionList[i].WelfareAmount;

I am looking into making this easier to do as well,  that just looks messy

Organization

The more that I look at your code, there are more things that I would change.

your Deduction and Earning objects are created and maintained by your WageInfo class  so really you should be using a wageInfoList  and each item in that list will have one Deduction object and one Earning object

This means that the WageInfo object should have all the information to hand off to the WageManager.  The WageManager shouldn't directly see the Deduction or Earning objects that live in the WageInfo Object that is called by the WageManager.


Saves files and attachments regardless of errors in the model I have a long form with dozens of fields, and users can upload photos and attachments in the form. It's super annoying when there is a simple field missing, the record doesn't save, and the user has to go and pick & choose all the files again. So I save the photos and attachments even upon validation errors. I would have liked to use asynchronous Javascript uploading, but that requires a separate form, and forms cannot be embedded in other forms.

This works, but it doesn't feel very DRY or very Rails. Is there a way to improve it?

class User < ApplicationRecord
  belongs_to :image, optional: true, dependent: :destroy
  belongs_to :company, optional: true, dependent: :destroy
  accepts_nested_attributes_for :company, :image

class Company < ApplicationRecord
  belongs_to :image, optional: true, dependent: :destroy
  has_and_belongs_to_many :attachments, dependent: :destroy
  accepts_nested_attributes_for :image, :attachments, :principals, reject_if: :all_blank, allow_destroy: true

class UsersController < ApplicationController
  def update
    @user = User.find(params[:id])
    @user.assign_attributes user_params # has to come before update_* or else it will create company twice for new users
    update_image
    update_company_image
    update_company_attachments
    if @user.save

  def update_image
    if image_params.has_key? :file
      if @user.image
        @user.image.update image_params
      else
        @user.create_image image_params
        @user.save(validate: false)
      end
    end
  end

  def update_company_image
    if company_image_params.has_key? :file
      if @user.company && @user.company.image # company & image exists
        @user.company.image.update company_image_params
      elsif @user.company # company exists
        @user.company.create_image company_image_params
        @user.company.save(validate: false)
      else
        @user.build_company
        @user.company.create_image company_image_params
        @user.company.save(validate: false)
        @user.save(validate: false)
      end
    end
  end

  def update_company_attachments
    company_attachments_params.each do |cap|
      if cap.has_key? :file
        if @user.company
          @user.company.attachments.create cap
        else
          @user.build_company
          @user.company.attachments.create cap
          @user.company.save(validate: false)
          @user.save(validate: false)
        end
      end
    end
  end

  def user_params
    params.require(:user).permit :name, ...,
      company_attributes: [:id, :name, ...,
        attachments_attributes: [:id, :_destroy]]
        # image_attributes: [:id, :file],
        # attachments_attributes: [:id, :file, :_destroy]]
      #image_attributes: [:id, :file]
  end

  def image_params
    params.require(:user).fetch(:image_attributes, {}).permit :id, :file
  end

  def company_image_params
    params.require(:user).fetch(:company_attributes, {}).fetch(:image_attributes, {}).permit :id, :file
  end

  def company_attachments_params
    params.require(:user).fetch(:company_attributes, {}).permit(:attachments_attributes, attachments_attributes: [:id, :file, :_destroy]).fetch(:attachments_attributes, {}).values
  end

Uploaded files are a tricky thing. You really don't want to move the files into their final resting spot until the other stuff is saved. Ideally, you want:

Commit to database
Move files to permanent storage
(Optionally) One more commit to the database to update file paths or names

Validation errors, of course, throw this all out of whack. From a UI perspective you solve this by switching your form from "create mode" to "edit mode" -- don't show the "upload" field. Show the files in the UI as if you were editing something that already existed in file storage, except you are referencing a temp file on the server instead of a file in permanent storage.

So now the process becomes:

Run validations (they fail...)
Update a temporary copy of you model with the temp file paths

This is where a "view model" can be useful, so the object used to render things on screen does not have to adhere to the same constraints as your Active Record classes

Re-render the page so the user can correct the errors
User corrects errors
User submits form, including references to temp files
Commit to database
Move files to permanent storage
(Optionally) One more commit to the database to update file paths or names


Singleton model for modifying future .chart elements I have written a singleton module which is to be used for some future modifications on .chart elements. Modification functions, such as colorArgumentButtons() are called recursively so that I don't have to iterate over all .chart elements in all functions.

Is my use of _ for private "class vars" proper? Otherwise they may conflict with my local vars.

Am I using the revealing module pattern correctly and is my namespace implementation okay? Feel free to comment on bad design choices!

var SuperHappy = (function (_parent, $) {

    var _my = _parent.Chart = _parent.Chart || {};

    // default settings
    var _settings = {
        autoFind: true,
        chartSelector: '.chart'
    };

    // jQuery vars
    var _$charts = $();

    _my.init = function (options) {
        /// <summary>
        /// Initializes the module.
        /// </summary>
        /// <param name="options">Custom JSON options</param>

        _settings = $.extend(_settings, options);

        if (_settings.autoFind) {
            _my.findCharts();
        }
    };

    var _applyFunctionToAll = function (func, $charts) {
        /// <summary>
        /// Applies the supplied function for each chart element in the collection.
        /// </summary>
        /// <param name="func">Function to be applied</param>
        /// <param name="$charts">Optional chart collection to apply function on</param>

        if (typeof $charts === 'undefined' && _$charts !== $()) {
            $charts = _$charts;
        }

        if ($charts.length === 1) {
            return false;
        }

        $charts.each(function () { // can this be converted to a for loop?
            func($(this));
        });

        return true;
    };

    _my.colorArgumentButtons = function (color, $chart) {
        /// <summary>
        /// Colors all argument buttons.
        /// </summary>
        /// <param name="color">Color to be used</param>
        /// <param name="$chart"></param>

        if (_applyFunctionToAll(_my.colorArgumentButtons.bind(null, color), $chart)) {
            return;
        }

        $chart.find('button').css('background-color', color);
    };

    _my.findCharts = function (selector) {
        /// <summary>
        /// Finds all chart elements.
        /// </summary>
        /// <param name="selector">Optional CSS selector for finding chart elements</param>

        selector = typeof selector !== 'undefined' ? selector : _settings.chartSelector;
        _$charts = $(selector);
    };

    return _parent;

})(SuperHappy || {}, jQuery);

jQuery(document).ready(function ($) {
    SuperHappy.Chart.init();
    SuperHappy.Chart.colorArgumentButtons('#f00');
});

http://jsfiddle.net/JrwRN/

I think you are overdoing the underscores, you will find that conflicts will be rare.

Furthermore, I can understand that you want to centralize the iteration over the chart elements, but why would you want to do that recursively, it does not make much sense to me.

If you drop underscores and opt out of recursive iteration, your iterator becomes simpler:

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    $charts.each(function (){ f($(this)); });
  };

This could be converted to a for loop as per your comment

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    for( var i = 0, length = $charts.length; i < length ; i++ ){
      f( $charts[i] );
    }
  };

Then, colorArgumentButtons becomes simpler as well:

  // Colors all argument buttons.
  my.colorArgumentButtons = function (color)
  {
    applyFunction( function( $chart )
    {
      $chart.find('button').css('background-color', color);
    });
  };

You will notice that I reduced the comments to a one line comment. When you have almost as much lines of comment as code, then you are doing it wrong.

A minor side note on

selector = typeof selector !== 'undefined' ? selector : settings.chartSelector;

this could simply be

selector = selector || settings.chartSelector;

All in all, I refactored the code into this ( untested ):

var SuperHappy = (function (parent, $) {

  var my = parent.Chart = parent.Chart || {},
      // default settings
      settings = {
        autoFind: true,
        chartSelector: '.chart'
      },
      // jQuery vars
      $charts = $();

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    $charts.each(function (){ func($(this)); });
  };

  my.init = function (options)
  {
    settings = $.extend(settings, options);
    if (settings.autoFind) {
      my.findCharts();
    }
  };

  /// Colors all argument buttons.
  my.colorArgumentButtons = function (color)
  {
    applyFunction( function( $chart )
    {
      $chart.find('button').css('background-color', color);
    });
  };

  // Finds all chart elements.
  my.findCharts = function (selector)
  {
    selector = selector || settings.chartSelector;
    $charts = $(selector);
  };

  return parent;

})(SuperHappy || {}, jQuery);

jQuery(document).ready(function ($) {
    SuperHappy.Chart.init();
    SuperHappy.Chart.colorArgumentButtons('#f00');
});


CacheManager - Dependency injection I create a cache manager to manage a system of cache exchangeable by another implementation.

I use the CacheManager class throughout my program. If I want to change the implementation I simply add a provider and I add it in my dependency container

Is it correct to do this like that?

ICache

namespace Caching
{
    public interface ICache
    {
        object Get(string key);

        void Set(string key, object value);
    }
}

ICacheManager

namespace Caching
{
    public static class CacheManager
    {
        private static ICache cache;
        private static readonly object syncRoot = new object();

        private static ICache Cache
        {
            get
            {
                lock (syncRoot)
                {
                    if (cache == null)
                    {
                        cache = Container.Instance.Resolve<ICache>();
                    }
                }

                return cache;
            }
        }

        public static object Get(string key)
        {
            return Cache.Get(key);
        }

        public static void Set(string key, object value)
        {
            Cache.Set(key, value);
        }
    }
}

MemoryCacheProvider

using System.Runtime.Caching;

namespace Caching.Providers
{
    public class MemoryCacheProvider : ICache
    {
        public object Get(string key)
        {
            return MemoryCache.Default.Get(key);
        }

        public void Set(string key, object value)
        {
            MemoryCache.Default.Add(key, value, DateTime.Now.AddMinutes(5));
        }
    }
}

Main

static void Main(string[] args)
{
    Container.Instance.RegisterType<ICache, MemoryCacheProvider>();
}

You are not doing dependency injection but using another pattern which is called service locator which isn't by the way so cool becasue classes using it depend on the Container:

cache = Container.Instance.Resolve<ICache>();

The CacheManager resolves an ICache from the Container. None of your classes should even know that the Container exists (but the one that configures it).

You should remove the CacheManager entirely and instead correctly inject the ICache to classes that require it.

class ThisClassRequiresCache
{
    public ThisClassRequiresCache(ICache cache) { .. }
}

Your dependency injection framework should handle the injection.

As far as the private Cache properety is concerned you should add another cache == null check to prevent unnecessary locks after the cache has already been created:

get
{
    if (cache == null)
    {
        lock (syncRoot)
        {
            if (cache == null)
            {
                cache = Container.Instance.Resolve<ICache>();
            }
        }
    }

    return cache;
}


Message queue with multiple producer, single consumer and no duplicates I had to make a message queue in an android app which my thread takes messages from and processes them. There can be multiple producers and duplicate messages should be avoided. Rate of insertion is erratic but not fast. Rate of consumption is slow. I came up with the following :

public class MessageQueueHolder<T> {

    public MessageQueueHolder(int initialSize) {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap<T, Boolean>());
    }
    public MessageQueueHolder() {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap<T, Boolean>());
    }
    private final Set<T> messageSet;

    /**
     * @param message to be inserted
     * @return if message already present returns false
     */
    public boolean insertMessage(T message) {
        return messageSet.add(message);
    }

    /**
     * @return returns the optional message and removes it
     */
    public Optional<T> takeMessage() {
        final Iterator<T> iterator = messageSet.iterator();
        if(!iterator.hasNext())
            return Optional.absent();
        final T message = iterator.next();
        iterator.remove();
        return Optional.of(message); //message can't be null
    }

    /**
     * Clear the message set
     */
    public void clear() {
        messageSet.clear();
    }
}

It seems to work just fine but I would appreciate some feedback :)

You code looks like it works just fine, but only because you have a single consumer thread. If you have multiple consumers, then you have a race-condition in your takeMessage method (a different thread may empty the queue after you call hasNext() but before you call next() causing an iteration exception....).

The whole iterator method also feels... clunky, and is a result of the basic Set usage.

I can't help but think that, in your situation, the use of the ConcurrentMap backing the set, is overkill. I know that HashSets have a backing Map as well, but that's different.

So, having thought some more about it as well, I also have to question the need to de-duplicate the producer messages as well in the Set... are you sure you need to?

Also, since you have the system you do, are you sure the queue will ever be big enough to worry about performance of the Set?

I can't help but feel you should go one of two ways:

Just use a LinkedTransferQueue and deal with duplicates elsewhere (use the add and poll methods respectively).
Just use a List with a simple contains() and basic synchronization.

Some other notes....

Why do you have a constructor that takes an initial size, but then does nothing?

Here's a simple synchronized version:

public class MessageClassHolder<T> {

    private final LinkedList<T> messages;

    public MessageClassHolder() {
        messages = new LinkedList<>();
    }

    /**
     * @param message to be inserted
     * @return if message already present returns false
     */
    public boolean insertMessage(T message) {
        synchronized(messages) {
            if (messages.contains(message)) {
                return false;
            }
            return messages.add(message);
        }
    }

    /**
     * @return returns the optional message and removes it
     */
    public Optional<T> takeMessage() {
        synchronized(messages) {
            return messages.isEmpty() ? Optional.empty() : Optional.of(messages.removeFirst());
        }
    }

    /**
     * Clear the message set
     */
    public void clear() {
        synchronized(messages) {
            messages.clear();
        }
    }
}


Constructing an SQL expression from three parameters I've been on stackoverflow looking for an alternative to the ugly if-elif-else structure shown below. The if-else structure takes three values as input and returns a pre-formatted string used for an SQL query. The code works right now, but it is difficult to explain, ugly, and I don't know a better way to do re-format it.

I need a solution that is more readable. I was thinking a dictionary object, but I can't wrap my mind how to implement one with the complex if/else structure I have below. An ideal solution will be more readable.

def _getQuery(activity,pollutant,feedstock):

   if feedstock.startswith('CG') and pollutant.startswith('VOC'):
        pass

    if feedstock == 'FR' and activity == 'Harvest':
        return 'FR'

    elif feedstock == 'FR':
        return 'No Activity'

    elif(activity == 'Fertilizer' and pollutant != 'NOx' and pollutant != 'NH3'):
        return 'No Activity'

    elif(activity == 'Chemical' and pollutant != 'VOC'):
        return 'No Activity'

    elif( (feedstock == 'CS' or feedstock == 'WS')
        and
        (activity == 'Non-Harvest' or
        activity == 'Chemical')):
        return 'No Activity'

    elif( ( ( pollutant.startswith('CO') or pollutant.startswith('SO') )
                and
            ( activity == 'Non-Harvest' or
            activity == 'Harvest' or
            activity == 'Transport' )
            )
            or
            ( pollutant.startswith('VOC')
                and not
            ( feedstock.startswith('CG') or feedstock.startswith('SG') )
            )
        ):

        rawTable = feedstock + '_raw'
        return """
    with
    activitySum as (select distinct fips, sum(%s) as x
        from %s where description ilike '%s' group by fips),

    totalSum as (select distinct r.fips, sum(r.%s) as x
        from %s r group by r.fips),

    ratios as (select t.fips, (a.x/t.x) as x from activitySum a, totalSum t
        where a.fips = t.fips),

    maxRatio as (select r.fips, r.x as x from ratios r
        group by r.fips, r.x order by x desc limit 1),

    minRatio as (select r.fips, r.x as x from ratios r
        group by r.fips, r.x order by x asc limit 1)

    select mx.x, mn.x from maxRatio mx, minRatio mn;
    """ % (pollutant, rawTable, '%'+activity+'%',
       pollutant, rawTable)

    elif( (pollutant[0:2] == 'PM')
            and
            (activity == 'Non-Harvest' or
            activity == 'Harvest' or
            activity == 'Transport')):

        rawTable = feedstock + '_raw'
        return """
    with
    activitySum as (select distinct fips, (sum(%s) + sum(fug_%s)) as x
        from %s where description ilike '%s' group by fips),

    totalSum as (select distinct r.fips, (sum(r.%s) + sum(r.fug_%s)) as x
        from %s r group by r.fips),

    ratios as (select t.fips, (a.x/t.x) as x from activitySum a, totalSum t
        where a.fips = t.fips),

    maxRatio as (select r.fips, r.x as x from ratios r
        group by r.fips, r.x order by x desc limit 1),

    minRatio as (select r.fips, r.x as x from ratios r
        group by r.fips, r.x order by x asc limit 1)

    select mx.x, mn.x from maxRatio mx, minRatio mn;
    """ % (pollutant, pollutant, rawTable, '%'+activity+'%',
       pollutant, pollutant, rawTable)

.
.
.
Lots more complex elif statements

My thought would be to create a collection of objects that can be iterated over. Each object would have an interface like the following.

def _getQuery(activity,pollutant,feedstock):
  for matcher in MATCHERS:
    if matcher.matches(activity,pollutant,feedstock):
      return matcher.get_result(activity,pollutant,feedstock)
  raise Exception('Not matched')

Then you build up the set of matcher classes for the various cases and ad an instance of each to MATCHERS. Just remember that earlier elements in the list take precedence to later ones. So the most specific cases should be first in the list ans the most general cases should be at the end.

NOTE:
Your string building code is vulnerable to SQL-injection. If you do go forward with manually building your selection strings, your input needs to be more thoroughly sanitized.