Using dapper Unit of Work in 3-tier architecture The Unit of Work:

public class UnitOfWork : IUnitOfWork
{
    private IDbConnection _connection;
    private IDbTransaction _transaction;

    private bool _disposed;
    private _repository;
    private UnitOfWork(string connectionString)
    {
        _connection = new SqlConnection(connectionString);
        _connection.Open();
        _transaction = _connection.BeginTransaction();
    }

    public static IUnitOfWork Create(string connectionString)
    {
        return new UnitOfWork(connectionString);
    }
   public IRepository Repository
    {
        get { return _repository ?? (_repository = new Repository(_transaction)); }
    }

    public void Commit()
    {
        try
        {
            _transaction.Commit();
        }
        catch
        {
            _transaction.Rollback();
            throw;
        }
        finally
        {
            _transaction.Dispose();
            _transaction = _connection.BeginTransaction();
            resetRepositories();
        }
    }

    private void resetRepositories()
    {
        _repository = null;
    }

    public void Dispose()
    {
        dispose(true);
        GC.SuppressFinalize(this);
    }

    private void dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                if (_transaction != null)
                {
                    _transaction.Dispose();
                    _transaction = null;
                }
                if (_connection != null)
                {
                    _connection.Dispose();
                    _connection = null;
                }
            }
            _disposed = true;
        }
    }

    ~UnitOfWork()
    {
        dispose(false);
    }
}

It's not my code. I took it from Tim Schreiber repository. I need transactions for add/update operations in my project.
And I use the Unit of Work in business logic assembly like this:

public class DealService : Service, IDealService
{
    public DealService(string connectionString)
        : base(connectionString)
    {

    }

    public async Task<User> GetUserAsync()
    {
        var user = null;
        using (var uow = UnitOfWork.Create(_connectionString))
        {
            user = await uow._repository.GetFirstAsync();
        }
        throw new NotImplementedException();
        //business logic here.
    }
}

Service.cs is an abstract class that has protected field _connectionString. And DI composition root I have in ASP.NET Core layer:

public IConfigurationRoot Configuration { get; }

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddMvc();
        string connectionString = Configuration.GetConnectionString("default");

        services.AddTransient<IDealService, DealService>(service => new DealService(connectionString));

    }

I'm feeling that I do something wrong so I have several questions:

As you can see in current realization I have to pass connection string to each Service and then pass to UnitOfWork constructor. Is it right way or exists solution where I can set connection once?
I use Unit Of Work without injection and don't like it. But how can I do injection if I need to use Dispose()?

I will be very glad to any help and advice.

You're correct in point #1 that passing the connection string to each service to get your UoW instance is bad form.  I'm not sure if that code was designed for a DI first world as is the case Asp.Net Core.

My recommendation (for #2) is keep it simple.  Remove the Unit of Work shell, but keep the repository contained inside.  Repository methods are usually transactional, and according to your question, you're using Dapper, so maintaining transactional consistency can be done per method call (not based on state as with the UoW).

SQL transactions are part of the natural semantics of Dapper:

public static IEnumerable<T> Query<T>(this IDbConnection cnn, string sql, object param = null, SqlTransaction transaction = null, bool buffered = true)

So your repository methods have the rough form of:

(assuming an open connection)

using(var tran = connection.BeginTransaction()) {
    try {
        // multiple operations involving connection and tran here

        tran.Commit();
    } catch {
        tran.Rollback();
        throw;
    }
}

The Dispose() is implicit to the using and you've neutralized your issue.

See: using Statement

If you absolutely need UoW style, you may consider moving to something like Entity Framework (or a heavier weight ORM in general), as the DBContext is essentially a much better featured and maintainable unit of work design, if not overkill for your project.


CompletableFuture Chain (Monad) I wrote an application that has a modular structure. Each module communicates with different remote servers. Time for execution of a task in each module varies. So communication should be asynchron.

Plus some restrictions like flexibility, scalability and so on are put.

So i decided to use CompletableFutures to build a composition of functions (monade), that modules execute.

Each module has its own FixedThreadPool und a public function, that create and sends back a CompletableFuture. Something like that:

private final ExecutorService executorService = Executors.newFixedThreadPool(10);

public CompletableFuture<String> function1(final String rawReport, final UUID id) {
     LOG.info("SEND a report for " + id);
     return CompletableFuture.supplyAsync(() -> run(rawReport, id), this.executorService);
}

private String run(final String rawReport, final UUID id) {
     // do something
}

The whole process through all modules is handled by ModuleChainHandler:

 public CompletableFuture<Result> handler(final RunnerJob job) {
        // @formatter:off
        return this.scanExecuterRegistry.function1(job).
                thenCompose(bArr -> this.module2.function2(bArr, job)).
                thenCompose(metrics -> this.module3.function3(metrics, job)).
                handle((vReport, throwable) -> {
                    if (vReport != null) {
                        this.module4.function4(vReport, job);
                        return builder().setResult(vReport).build();
                    } else {
                        return builder().setErrorMessage(throwable.getMessage()).build();
                    }
                });
        // @formatter:on
    }

The BIGGEST problem (at the beginning) was to add an error handling. Because you cant throw an exception directly out of a future. I had to create an object with two state (result and error). If a completable future completes exceptionally, will be one action executed and the result object has an error massage and an empty result.

There is no so much real examples in internet except some poor tutorials. So just interesting, if you do it in the same way.

One thing that i'd like to propose is to replace CompletableFuture with home-brewed builder class that wraps CompletableFuture but will have more 'easy to read' interface. It would be something like this:

class FutureBuilder {

/**
 * Wrapped 'completable future' instance
 */
CompletableFuture future;

...
public FutureBuilder execute(Function<...> task) {...}
public FutureBuilder whenComplete(Consumer<Report> handler) {...}
public FutureBuilder whenFail(BiComsumer<Report, Throwable> handler) {...}
...

/**
 * This method should be called at the end to meet interface requirements
 */
public CompletableFuture build() {
      return future;
}
...
}

So, it may be used in next way:

public CompletableFuture<Result> handler(final RunnerJob job) {
   return new FutureBuilder ()
              .execute(bArr -> this.module2.function2(bArr, job))
              .execute(metrics -> this.module3.function3(metrics, job))
              .whenComplete(r-> uilder().setResult(r).build())
              .whenFail((r, e) -> {
                   this.module4.function4(r, job);
                   return builder().setErrorMessage(e.getMessage()).build();
              })
              .build();
}

I hope that is you are looking for.


Generating strings dynamically in Python I'm generating a URL (in string) that depends on 3 optional parameters, file, user and active.

From a base url: /hey I want to generate the endpoint, this means:

If file is specificied, my desired output would is: /hey?file=example
If file and user is specified, my desired output is: /hey?file=example&user=boo
If user and active are specified, my desired output is: /hey?user=boo&active=1
If no optional parameters are specified, my desired output is: /hey
and so on with all the combinations...

My code, which is working correctly, is as follows (change the None's at the top if you want to test it):

file = None
user = None
active = 1

ep = "/hey"
isFirst = True

if file:
  if isFirst:
    ep+= "?file=" + file;
    isFirst = False;
  else: ep += "&file=" + file;

if user:
  if isFirst:
    ep+= "?user=" + user;
    isFirst = False;
  else: ep += "&user=" + user;

if active:
  if isFirst:
    ep+= "?active=" + str(active);
    isFirst = False;
  else: ep += "&active=" + str(active);

print ep

Can someone give me a more python implementation for this? I can't use modules as requests.

Thanks in advance.

You're pretty much reinventing urllib.parse.urlencode:

from urllib.parse import urlencode

def prepare_query_string(**kwargs):
    return urlencode([(key, value) for key, value in kwargs.items() if value is not None])

Usage being:

>>> prepare_query_string(active=1)
'active=1'
>>> prepare_query_string(active=1, user=None)
'active=1'
>>> prepare_query_string(active=1, user='bob')
'active=1&user=bob'
>>> prepare_query_string(file='foo.tar.gz', user='bob')
'file=foo.tar.gz&user=bob'
>>> prepare_query_string(file='foo.tar.gz', user='bob', active=None)
'file=foo.tar.gz&user=bob'
>>> prepare_query_string(file='foo.tar.gz', user='bob', active=1)
'file=foo.tar.gz&user=bob&active=1'


Autocompleting console input Yesterday I stumbled upon a Stack Overflow question that asked if it was possible to implement Tab-triggered auto-completion in a console application.

I thought that was an interesting idea, so I went ahead and wrote a small program that does just that, given a hard-coded string[] - I might eventually refactor it into its own class and reuse it in my own projects if I ever actually need auto-completion in a console app, but before I do that I'd like some feedback on the way it's implemented, given static aside, the logic itself is pretty much the way I'll have it regardless of whether it's in a dedicated class as part of something much bigger, or right there in a console application that does nothing but verify that the code works.

Pressing Tab when there's no input will change the current line to say Bar; because there's a word in data that starts with Bar, pressing Tab again will autocomplete to Barbec; another Tab will make it Barbecue, and then any subsequent Tab will have no effect, because nothing in the data starts with Barbecue - but then you could Backspace until the input is Ba, type a t to make it Bat, and when you press Tab then it autocompletes to Batman.

In other words, it all works exactly as it should. But does it look right?

class Program
{
    static void Main(string[] args)
    {
        var data = new[]
        {
            "Bar",
            "Barbec",
            "Barbecue",
            "Batman",
        };

        var builder = new StringBuilder();
        var input = Console.ReadKey(intercept:true);

        while (input.Key != ConsoleKey.Enter)
        {
            if (input.Key == ConsoleKey.Tab)
            {
                HandleTabInput(builder, data);
            }
            else
            {
                HandleKeyInput(builder, data, input);
            }

            input = Console.ReadKey(intercept:true);
        }
        Console.Write(input.KeyChar);
    }

    /// <remarks>
    /// https://stackoverflow.com/a/8946847/1188513
    /// </remarks>>
    private static void ClearCurrentLine()
    {
        var currentLine = Console.CursorTop;
        Console.SetCursorPosition(0, Console.CursorTop);
        Console.Write(new string(' ', Console.WindowWidth));
        Console.SetCursorPosition(0, currentLine);
    }

    private static void HandleTabInput(StringBuilder builder, IEnumerable<string> data)
    {
        var currentInput = builder.ToString();
        var match = data.FirstOrDefault(item => item != currentInput && item.StartsWith(currentInput, true, CultureInfo.InvariantCulture));
        if (string.IsNullOrEmpty(match))
        {
            return;
        }

        ClearCurrentLine();
        builder.Clear();

        Console.Write(match);
        builder.Append(match);
    }

    private static void HandleKeyInput(StringBuilder builder, IEnumerable<string> data, ConsoleKeyInfo input)
    {
        var currentInput = builder.ToString();
        if (input.Key == ConsoleKey.Backspace && currentInput.Length > 0)
        {
            builder.Remove(builder.Length - 1, 1);
            ClearCurrentLine();

            currentInput = currentInput.Remove(currentInput.Length - 1);
            Console.Write(currentInput);
        }
        else
        {
            var key = input.KeyChar;
            builder.Append(key);
            Console.Write(key);
        }
    }
}

Testing

Personally I think it’s conceptually easier to test this code if you are returning a string – rather than a void: what if you decided to change the out put device from a console window to your printer – it would not be easy to change that. What if sometimes you wanted a console window and other times you wanted to send the same thing via text message? I guess it’s easier to separate what is being done vs how it is being done. E.g. Output key is the abstract concept but this could be done a number of different ways: (i) to the console, or (ii) through a voice synthesiser etc. I wrote some tests but then i threw it once it broke when i changed the API.

Naming

What is data? It’s harder to think of a more abstract name. I’ve changed this to keywords.
Personally I like to see the type along with the variable declaration.

Refactoring the code:

I simplified and extracted some methods.
The The HandleKeyInput and the HandleTabInput both seem to be doing conceptually similar things: a key is inputted and then a result is obtained. If it’s a tab then do X, but if it’s a key then do Y. There’s a common abstraction here that can be extracted.
What I want to do is to make the two methods: HandleTabInput and HandleKeyInput to look and be absolutely identical.
Data (renamed to keywords) can be a field so there’s no need to pass it in as a variable.
I want to make two of your methods to basically be the same. They’re kinda doing this same things. After some refactoring this is what I got:

public void HandleTabInput(StringBuilder builder, ConsoleKeyInfo keyInput)
{
        // Perform calculation
    string match = ExtractMatch(builder);

        // Alter the builder
    builder.Clear();
    builder.Append(match);

        // Print Results
    PrintMatch(match);
}

And then this:

    private void HandleKeyInput(StringBuilder builder, ConsoleKeyInfo keyInput)
    {
        if (keyInput.Key == ConsoleKey.Backspace && builder.ToString().Length > 0)
        {
                // Perform Calculation (nothing here)

                // Alter the builder
            builder.Remove(builder.Length - 1, 1);

                // Print Results
            ClearCurrentLine();
            Console.Write(builder.ToString().Remove(builder.ToString().Length - 1));
        }
        else
        {
                // Perform calculation (nothing here)

                // Alter the Builder
            var key = keyInput.KeyChar;
            builder.Append(key);

                // Print Reuslts
            Console.Write(key);
        }
    }

We can abstract out the calculation, and then the printing. After some work this was the result. It’s starting to look pretty similar:

    private void HandleKeyInput(StringBuilder builder, ConsoleKeyInfo keyInput)
    {
        if (keyInput.Key == ConsoleKey.Backspace && builder.ToString().Length > 0)
        {
            Program.KeyInput backSpaceKey = new Program.KeyInput.BackspaceInput(builder, keywords);
            backSpaceKey.AlterBuilder();
            backSpaceKey.PrintResult();
        }
        else
        {
            KeyInput input = new KeyInput.StandardKeyInput(builder, keywords, keyInput.KeyChar);
            input.AlterBuilder();
            input.PrintResult();
        }
    }

You could probably tidy things up a little more.

The final Result:

    public void RunProgram()
    {
        StringBuilder builder = new StringBuilder();
        ConsoleKeyInfo capturedCharacter = Console.ReadKey(intercept: true);

        while (EnterIsNotThe(capturedCharacter))
        {
            KeyInput key = KeyInput.GetKey(builder, keywords, capturedCharacter);
            builder = key.UpdateBuilder();
            key.Print();

            capturedCharacter = Console.ReadKey(intercept: true);
        }

        Console.Write(capturedCharacter.KeyChar);
    }

Concluding Thoughts

It reads marginally better. Still If you want to reuse the code you’ll have to make changes, but at least it will be a little easier to do when the time comes.
Encapsulation could be a little better – typically speaking you want the KeyInput class to be instantiated via the factory only.

A link to see the full code.


Search insert position I am trying to solve this problem:

  Given a sorted array and a target value, return the index if the target is found. If not, return the index where it would be if it were inserted in order.

and this is my implementation:

public int searchInsert(int[] numbers, int target) {
    int startIndex = 0;
    int endIndex = numbers.length - 1;
    int midIndex;
    while (startIndex <= endIndex) {
        midIndex = (endIndex - startIndex) / 2 + startIndex;
        if(numbers[midIndex] == target)
            return midIndex;
        else {
            if(numbers[midIndex] > target) {
                if(midIndex <= 0 || numbers[midIndex - 1] < target) {
                    return midIndex;
                }
                endIndex = midIndex - 1;
            }
            else {
                if(midIndex >= numbers.length - 1 || numbers[midIndex + 1] > target) {
                    return midIndex + 1;
                }
                startIndex = midIndex + 1;
            }
        }
    }
    return -1;
}

How can I make it more efficient?

In Java, the most efficient implementation of this will likely be

public int searchInsert(int[] numbers, int target) {
    int index = Arrays.binarySearch(numbers, target);

    if (index < 0) {
        index = -index - 1;
    }

    return index;
}

With that said, the place where I'd look first are your special cases.  Under what circumstances would midIndex be less than 0 or more than numbers.length - 1?  Never (if it was, numbers[midIndex] would throw an out of bounds exception).  And when will it be equal?  When we've found the edge, when endIndex will be less than startIndex.

So simplify

    while (startIndex < endIndex) {
        midIndex = (endIndex - startIndex) / 2 + startIndex;
        if (numbers[midIndex] == target) {
            return midIndex;
        }

        if (numbers[midIndex] < target) {
            startIndex = midIndex + 1;
        }
        else {
            endIndex = midIndex - 1;
        }
    }

    return startIndex;

This will do one extra assignment and outer loop comparison but save two comparisons on every iteration.  Since the assignment only involves registers and math, it should be quick (possibly quicker than the comparisons, which have to load the registers from cache if not memory).

Without the extra checks, the code will update either startIndex or endIndex and make the same return.  Because startIndex will equal either midIndex or midIndex + 1, the same as was returned in the original code.


Predicting outliers in network data I'm implementing KNN algorithm to predict outliers in network data which contains the following columns: source IP address, source port number, protocol and total bytes transferred. To achieve this, I'm using the Python library sklearn to predict response variable (total bytes) with regards to explanatory variables (source IP address, source port number, protocol).

However, the accuracy of prediction on test data is extremely low (0.21). Any recommendations to improve the accuracy score will be much appreciated.

import pandas as pd
from sklearn.neighbors import KNeighborsRegressor
import random
import numpy as np
from sklearn.model_selection import train_test_split
from sklearn.neighbors import KNeighborsClassifier
from numpy.random import permutation
from sklearn.metrics import accuracy_score
import sys
import math
import netaddr as nt
from netaddr import IPAddress

#Import csv into dataframe
df = pd.read_csv('data.csv')
df = df[['sourceaddress','sourceport','protocol','total_bytes']]

df_arr = df.as_matrix()

for i in range(df_arr.shape[0]):
    df_arr[i,0] = int(nt.IPAddress(df_arr[i,0]))

df = pd.DataFrame(df_arr)
df.columns = ['sourceaddress','sourceport','protocol','total_bytes']
df = df.astype('str')

# Design matrix X and target vector y
X = np.array(df.ix[:, 0:3])
Y = np.array(df['total_bytes'])

# split into train and test
X_train, X_test, Y_train, Y_test = train_test_split(X, Y, test_size=0.33, random_state=42)

knn = KNeighborsClassifier(n_neighbors=24)
# fitting the model
knn.fit(X_train, Y_train)

# prediction of the response variable
pred = knn.predict(X_test)
print pred

# evaluate accuracy
print accuracy_score(Y_test, pred)

You mention accuracy, but talk about the number of bytes, so I'm going to assume that you apply a cut-off point to number of bytes and convert it to binary variable behind the screen. Now the answer:

The reason why the performance is so low is most probably lack of feature engineering. You should definitely inject as much domain knowledge as possible. Let's go over it step by step:

You are, if I understand correctly one-hot encoding IP addressees - however, it could be done in more concise and pythonic way without casting dataframe to matrix:

dummies = pd.get_dummies(df['sourceaddress'], prefix='ip')  # also called one-hot encoding
df = df.merge(dummies, left_index=True)

Protocol should be one-hot encoded as well, as it is a categorical variable.

Now you might want to group IPs by geographic region - this will allow the algorithm to pick up any correlations between not only particular IP but a country or continent. That's how you can add such a feature:

df['region'] = determine_region([df['sourceaddress'])

If you write determine_region(ip) in a way that allows array-wide operations (in other words using numpy), it will be applied to whole dataset at once. This vectorization tends to be faster than .apply, especially for complex functions. And you should also one-hot encode the regions.

Same probably applies to sourceport (not an expert on web technology, so making assumptions here) - there are certain ports that are used in general for some thing and not for others, so another feature that reflects that could be crafted and then one-hot encoded (all your features are categorical so far, tough luck).

Now, speculating here, but maybe combination of region and sourceport is meaningful as well? Really think about anything and everything that might explain the value of your target and create a feature out of that. Then you could choose the most relevant features, using one of the feature selection techniques, but that's not nearly as important as creating features in the first place.
Next, I expect that your dataset is highly imbalanced, which means that your algorithm will be biased towards majority class. What is typically done is rebalancing. Depending on the size of your dataset, you either upsample the minority class (with replacement obviosuly), or downsample the majority class (without replacement, that's important). An example of downsampling when majority class is ~total_bytes - so normal amount of bytes:

df_majority = df[~df['total_bytes']]
df_minority = df[df['total_bytes']]
df_majority_downsampled = resample(df_majority,
                               replace=False,
                               n_samples=len(df_minority),
                               random_state=0)
df_downsampled = pd.concat([df_majority_downsampled, df_minority])

Now the amount of positive and negative examples is equal, so no bias.

Then you should try to tune the hyper parameters. Good way to do that is gridsearch with crossvalidation. Docs are excellent, so no need to explain further here.

Doing all this should improve the performance of your model.


TinyMVC Model / Plugin how to implement? Recently I started working with TinyMVC, wrote a simple menu model and I have few questions for those who are using it... or used before.

For the following code should I keep it as Model or as a Plugin?
How should I implement it in the view and use it on every page that is required without breaking the ideea of MVC and without rewriting again and again for each controller?
Any improvements to the code?
Need the MySQL tables?

<?php
 class Menu_Model extends TinyMVC_Model
{

    public function __construct()
    {
        parent::__construct();
    }

    public function listMenu()
    {
        return $this->db->query_all("SELECT * FROM menu_links WHERE is_deleted = 0 ORDER BY position");
    }

    public function listCategorys($menuLinkId)
    {
        return $this->db->query_all("SELECT * FROM menu_subcategorys WHERE menuLinkId = ? AND is_deleted = 0 ORDER BY position", array($menuLinkId));
    }

    public function buildMenu()
    {
        $this->listMenu = $this->listMenu();
        foreach($this->listMenu as $this->listMenuKey => $this->listMenuValue)
        {
            $this->listCategorys = $this->listCategorys($this->listMenuValue['menuLinkId']);
            if(!empty($this->listCategorys))
                $this->listMenu[$this->listMenuKey]['child'] = $this->listCategorys;
        }

        return $this->listMenu;
    }
}

Sorry you had to wait so long, answers here tend to take some time and I've been on vacation the last week, so I wasn't keeping up with this site the way I usually do. So, here we go...

Q: "Any improvements to the code?"

First off, I don't think you quite understand inheritance. If you are extending one class from another it automagically inherits all properties and methods of the parent class. Therefore, there is no need for you to recreate a __construct() method if all you are going to do with it is call the parent one. The only reason to do it the way you have is if you had called additional code in the new constructor before or after calling the parent one. Example:

public function __construct() {
    $changedParentVar = 'new value';
    parent::__construct();
    echo 'we changed the "changedParentVar" inside the parent constructor method before running it';
}

Clarification: I'll be honest, I'm not quite sure if this stands true for deep inheritance. Say I have three classes, each inherits from the previous, and only the first has a constructor. I don't know if the third will still have the original constructor. I know it will still have all the other methods, but for some reason I want to say that magical methods are different, yet I can't find anything at the moment to prove this one way or another. So this statement is here to cover my ass :)

I'd find some way of combining your "list" methods, something like...

public function list($from, $where, $and = '') {
    if( ! empty($and)) { $and = "AND $and"; }

    return $this->db_query_all("
        SELECT *
        FROM $from
        WHERE $where
        $and
        ORDER BY position
    ");
}

If you are not going to be reusing a variable ($this->listMenu or $this->listCategorys or a foreach variable) there is no need to make them properties. The only reason you should make a variable a property is so that it can be used outside the current method. That being said, take a look at that foreach loop... There is no need to set the key/value pair as a class property as they will not be reused before they are reset, and I doubt you will reuse just the last one. This also stands true for those other two properties I mentioned, they should be variables, not properties. So the buildMenu() method should be rewritten like so.

public function buildMenu() {
    $listMenu = $this->listMenu(); //Keep this line how you have it if you are using listMenu in another method you did not include here...
    foreach($listMenu as $key => $value) {
        $listCategorys = $this->listCategorys($value['menuLinkId']);//BTW its categories not categorys... been bothering me...
        if( ! empty($listCategorys)) { $listMenu[$key]['child'] = $listCategorys; }
    }

    return $listMenu;
}

By the way, please, for everyone's sanity, always use braces {} around any statements, even if they are only one line. Its so much easier to read.

Q: How should I implement it in the view and use it on every page that is required without breaking the ideea of MVC and without rewriting again and again for each controller?

You should not implement it in the view, only in the controller. Views should only contain minimal PHP that helps output data.

I'm not sure what you mean about having to rewrite it, please clarify. In the mean time here are a few examples of how I would implement it.

If my controller is a class:

class Controller extends Menu_Model { }
//OR
class Controller {
    public function __construct() {
        include 'path/to/Menu_Model.php';
        $this->Menu_Model = new Menu_Model();
    }
}

If my controller is just a list of PHP functions or procedural:

include 'path/to/Menu_Model.php';
$Menu_Model = new Menu_Model();

Since you are using MVC, I would also suggest that you take a look at autoloading.

Q: For the following code should I keep it as Model or as a Plugin?

Not sure what you mean by plugin. Model should be fine though.


Shuffling multiple iterators in order I have a Python function w/ specified functionality, and two proposed approaches - which do you prefer, and why? (performance, simplicity, clarity, etc) (both use import random)

PROBLEM: Need a plain Python function that accepts an arbitrary number of iterables (tuples, lists, dictionaries), and returns them shuffled in the same order:

a = (1, 2, {3: 4}, 5)
b = [(5,6), [7,8], [9,0], [1,2]]
c = {'arrow': 5, 'knee': 'guard', 0: ('x',2)}

x, y, z = magic(a, b, c)
print(x, y, z, sep='\n')
# ({3: 4}, 1, 2)
# [[9, 0], (5, 6), [7, 8]]
# {0: ('x', 2), 'arrow': 5, 'knee': 'guard'}

The function must:

Return iterables shuffled in the same order (see above)
Accept any number of iterables
Preserve iterables types
Support nested iterables of any depth and type
Not shuffle nested elements themselves (eg. [7,8] above doesn't become [8,7])
Return iterables with length of shortest iterable's length w/o raising error (see above)
Return shuffled iterables in the same order they're passed in (see above)

NOTE: requires Python 3.7. Can work with Python 3.6- if excluding dicts, as dicts aren't ordered.

SOLUTION 1:

def ordered_shuffle(*args):
    args_types = [type(arg) for arg in args]                               # [1]
    _args      = [arg if type(arg)!=dict else arg.items() for arg in args] # [2]
    args_split = [arg for arg in zip(*_args)]                              # [3]
    args_shuffled = random.sample(args_split, len(args_split))             # [4]
    args_shuffled = map(tuple, zip(*args_shuffled))                        # [5]
    return [args_types[i](arg) for i, arg in enumerate(args_shuffled)]     # [6]

Explanation: img

SOLUTION 2:

def shuffle_containers(*args):
    min_length = min(map(len, args))
    idx = list(range(min_length))
    random.shuffle(idx)
    results = []
    for arg in args:
        if isinstance(arg, list):
            results.append([arg[i] for i in idx])
        elif isinstance(arg, tuple):
            results.append(tuple(arg[i] for i in idx))
        elif isinstance(arg, dict):
            items = list(arg.items())
            results.append(dict(items[i] for i in idx))
        else:
            raise ValueError(
                "Encountered", type(arg),
                "expecting only list, dict, or tuple"
            )
    return results

functools.singledispatch

functools library includes the singledispatch() decorator.  It lets you provide a generic function, but provide special cases based on the type of the first argument.

import functools
import random

@functools.singledispatch
def shuffle(arg, order):
    """this is the generic shuffle function"""

    lst = list(arg)
    return type(arg)(lst[i] for i in order)

@shuffle.register(dict)
def _(arg, order):
    """this is shuffle() specialized to handle dicts"""

    item = list(arg.items())
    return dict(item[i] for i in order)

def ordered_shuffle(*args):
    min_length = min(map(len, args))

    indices = random.sample(range(min_length), min_length)

    return [shuffle(arg, indices) for arg in args]

Usage:

a = (1, 2, {3: 4}, 5)
b = [(5,6), [7,8], [9,0], [1,2]]
c = {'arrow': 5, 'knee': 'guard', 0: ('x',2)}

ordered_shuffle(a, b, c)

Output:

[({3: 4}, 1, 2),
 [[9, 0], (5, 6), [7, 8]],
 {0: ('x', 2), 'arrow': 5, 'knee': 'guard'}]


Simplest way to write "FizzBuzz" in Rust Can you write an a simpler Rust fizzbuzz program than I have? Use my output or the spec:

  Write a program that prints the numbers from 1 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”.

to write your program. I want to see if it's possible to write an even simpler program.

fn main() {
    for i in 1..102 {
        match i {
            i if (i % 15 == 0) => { println!("{:?}", "FizzBuzz") },
            i if (i % 3 == 0) => { println!("{:?}", "Fizz") },
            i if (i % 5 == 0) => { println!("{:?}", "Buzz") },
            _ => { println!("{:?}", i) },
        }
    }
}

I think that match is better, you just do not know how to cook it ;)

fn main() {
    for i in 1..102 {
        match (i%3, i%5) {
            (0, 0) => println!("FizzBuzz"),
            (0, _) => println!("Fizz"),
            (_, 0) => println!("Buzz"),
            (_, _) => println!("{}", i)
        }
    }
}


Wrap a callback method in an awaitable method I'm using a .NET library which uses a pre-async/await type of asynchronicity. That is, it provides asynchronous (non-awaitable) method with a callback parameter.

I'm trying to write awaitable extension methods which will wrap some of those methods to make it easier to use with the async/await fashion but I'm not sure if this is the right approach.

Here's an example of one of those methods:

public void OAuthGetRequestTokenAsync(string callbackUrl, Action<FlickrResult<OAuthRequestToken>> callback)
{
    CheckApiKey();
    string url = //...

    Dictionary<string, string> parameters = OAuthGetBasicParameters();
    // [Code to add parameters]

    FlickrResponder.GetDataResponseAsync(this, url, parameters, (r) =>
    {
        var result = new FlickrResult<OAuthRequestToken>();
        if (r.Error != null)
        {
            if (r.Error is System.Net.WebException)
            {
                var ex = new OAuthException(r.Error);
                result.Error = ex;
            }
            else
            {
                result.Error = r.Error;
            }
            callback(result);
            return;
        }
        result.Result = FlickrNet.OAuthRequestToken.ParseResponse(r.Result);
        callback(result);
    });
}

And here's what I came up with to provide an awaitable extension method which wraps the one above:

static class FlickrExtensions
{
    public static Task<OAuthRequestToken> OAuthGetRequestTokenAsync(this Flickr flickr,
                                                                    string callbackUrl)
    {
        var tcs = new TaskCompletionSource<OAuthRequestToken>();
        flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
        {
            if (r.HasError)
                tcs.TrySetException(r.Error);
            else
                tcs.TrySetResult(r.Result);
        });

        return tcs.Task;
    }

    // More extension methods to be added.
}

Are there any problems with my code or any downsides to this implementation?

Also, is there a better (more standard?) way to achieve this goal?

Error Handling

You are relying on OAuthGetRequestTokenAsync to catch all exceptions for you.

flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
{
    if (r.HasError)
        tcs.TrySetException(r.Error);
    else
        tcs.TrySetResult(r.Result);
});

Unhandled errors

However, OAuthGetRequestTokenAsync internally calls  FlickrResponder.GetDataResponseAsync, which as you can see in the Reference Source does not catch all exceptions.

Most exceptions are caught and provided to the callback:

// .. snippet from FlickrNet.Internals.FlickrResponder.GetDataResponseAsync()
if (e.Error != null)
{
    result.Error = e.Error;
    callback(result);
    return;
}

But some are thrown to the caller:

// .. snippet from FlickrNet.Internals.FlickrResponder.GetDataResponseAsync()
using (var responseReader = new StreamReader(response.GetResponseStream()))
{
    string responseData = responseReader.ReadToEnd();

    throw new OAuthException(responseData, ex);
}

Refactored Code

To wrap this call in an async task, I would also catch these exceptions and handle them in the Task.TrySetException. In addition, since you provide a public API, I suggest to avoid NullReferenceException and check required arguments against null. Perhaps callbackUrl should also be checked early against null. I'm not sure about this, you'd have to verify.

public static Task<OAuthRequestToken> OAuthGetRequestTokenAsync(
    this Flickr flickr, string callbackUrl)
{
    if (flickr == null) throw new ArgumentNullException(nameof(flickr));
    var tcs = new TaskCompletionSource<OAuthRequestToken>();
    try
    {
        flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
        {
            if (r.HasError)
                tcs.TrySetException(r.Error);
            else
                tcs.TrySetResult(r.Result);
        });
    }
    catch (Exception uncaughtError)
    {
        tcs.TrySetException(uncaughtError);
    }

    return tcs.Task;
}


A simple and safe implementation for strnstr (search for a substring in the first n characters of char array) I'd like to suggest the following implementation:

// Find an instance of substr in an array of characters
// the array of characters does not have to be null terminated
// the search is limited to the first n characters in the array.
char *strnstr(char *str, const char *substr, size_t n)
{
    char *p = str, *pEnd = str+n;
    size_t substr_len = strlen(substr);

    if(0 == substr_len)
        return str; // the empty string is contained everywhere.

    pEnd -= (substr_len - 1);
    for(;p < pEnd; ++p)
    {
        if(0 == strncmp(p, substr, substr_len))
            return p;
    }
    return NULL;
}

The rationale for the first parameter is not to be const is that you may want to use the return value pointer to modify the array in that location.

for completeness, in C++, it's possible to add an overloaded variant that's const:

const char *strnstr(const char *str, const char *substr, size_t n)
{
    return strnstr((char *)str, substr, n);
}

Any comments?

As suggested, here's a test program:

#include <iostream>
#include <cstring>
#include <string>

int main()
{
    char s[] = "1234567890abcdefgh";
    size_t n = sizeof(s) - 1;

    const char *patterns[] = { "efgh", "0ab", "0b", NULL };
    const char *result = NULL;
    const char *pPattern = patterns[0];

    std::cout << "array of length " << n << " is: " << s << std::endl;

    for (int i = 0; pPattern; pPattern = patterns[++i])
    {
        result = strnstr(s, pPattern, n);
        std::cout << "finding " << pPattern << " n=" << n << ": "
            << (result ? result : "(null)") << std::endl;
    }

    pPattern = patterns[0];
    result = strnstr(s, pPattern, n-1);
    std::cout << "finding " << pPattern << " n=" << n-1 << ": "
        << (result ? result : "(null)") << std::endl;
    return 0;
}

Output:

array of length 18 is: 1234567890abcdefgh
finding efgh n=18: efgh
finding 0ab n=18: 0abcdefgh
finding 0b n=18: (null)
finding efgh n=17: (null)

Design:  The str as an array and searched up to n is inconsistent with string like functions and strstr().  Rather than "the search is limited to the first n characters in the array", I'd also expect characters in str that follow a null character are not searched.   IMO, a design flaw.

Following review assumes str[i] == 0 has no special meaning.
Weak argument name str.  str is the address of an array and maybe not a string.  Calling a potential non-string str conveys the wrong idea.  Suggest src, etc.  When looking for sub-strings, I like needle and haystack.
As the C version does not change str contents, recommend making that const.  "The rationale for the first parameter is not to be const is that you may want to use the return value pointer to modify the array in that location." does not apply.  Just cast char * the return value.  Follow strstr()s tyle.

// From C library.
      char *strstr( const char *s1, const char *s2);

// Expected signatures: (spaced for clarity)
// C
      char *strnstr(const char *src, const char *substr, size_t n);
// C++
      char *strnstr(      char *src, const char *substr, size_t n);
const char *strnstr(const char *src, const char *substr, size_t n);

Using a name that is close to standard names is tricky.  C reserves name with certain prefixes, etc and so does *nix, etc.  Maybe use CP_strnstr() and an optional #define strnstr CP_strnstr.
Corner case: Returning str with if(0 == substr_len) return str; does not make sense when size == 0.  I'd expect NULL.
Underflow possible.  The length of the needle may be longer or shorter than the haystack

// add check
if (n + 1 < substr_len) {
  return NULL;
}
pEnd -= (substr_len - 1);

Minor

In debug mode, consider testing against NULL

char *strnstr(char *str, const char *substr, size_t n) {
  assert(str || n == 0);
  assert(substr);


Interview street Median challenge I was trying to solve the Median challenge at Interviewstreet.com.

I wasn't able to pass most of the test cases. But according to my understanding my code should work fine.

Here is the question:
https://www.interviewstreet.com/challenges/dashboard/#problem/4fcf919f11817

Here is my code:

    /* Sample program illustrating input and output */

    import java.util.*;

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

     // helpers for input/output

    Scanner in = new Scanner(System.in);

    int N;
    N = in.nextInt();

    String s[] = new String[N];
    int x[] = new int[N];
    ArrayList<Integer> items=new ArrayList<Integer>();
    float result[]=new float[N];
    //int itemindex=-1;
    for(int i=0; i<N; i++){
        s[i] = in.next();
        x[i] = in.nextInt();

        switch(s[i].charAt(0))
        {

        case 'r':
            if(items.size()<=1)
            {
                result[i]=-1;

            }
            else
            {
                items.remove((Integer)x[i]);
                int itemindex=items.size();
                Collections.sort(items);
                if((itemindex)%2==0)
                {
                    result[i]=(float)         ((items.get((itemindex/2))+items.get(((itemindex)/2)-1))/2.0);

                }
                else
                {
                    result[i]=items.get((itemindex-1)/2);
                }

            }
            break;

        case 'a':
            items.add(x[i]);
            int itemindex=items.size();
            Collections.sort(items);
            if((itemindex)%2==0)
            {
                result[i]=(float)((items.get((itemindex/2))+items.get(((itemindex)/2)-1))/2.0);

            }
            else
            {
                result[i]=items.get((itemindex-1)/2);
            }

            break;

        }

    }

    for(int j=0;j<N;j++)
    {
        if(result[j]==-1)
        {
            System.out.println("Wrong!");
        }
        else
        {
            if((result[j]*10)%10==0)
            {
            System.out.println((int)(result[j]));
            }
            else
            {
                System.out.println(result[j]);
            }
        }

    }

}
    }

I would highly appreciate if someone could help me find out the test cases which will fail  for my code.

Thanks.

An additional point to the ones mentioned by @palacsint is the check for the object removal:
according to the challenge:

  If the operation is remove and the number x is not in the list, output
  "Wrong!" in a single line

your code does not check for that: here is what you have:

if(items.size()<=1)
            {
              result[i]=-1;
            }
else .....

You are marking wrong when the list has a single item or is empty. Obviously this is not what the challenge requires. Therefore, you should discard this "if" block and only check the return value from the remove on the list:

boolean ok= items.remove((Integer)x[i]);
if (!ok){
  result[i]=-1;
}


Find all occurrences of all permutations of a shorter string within a longer string Input is two strings, the first string being the long string and the second smaller string being the permuted string.

The output should return the starting indices of all occurrences as well as the permutation of the substring in the longer string.

Case 1)

permutationsInString('abcdcbadefbac', 'abc')
sample output: { '0': 'abc', '4': 'cba', '10': 'bac' }

Case 2)

 permutationsInString('abcabacba', 'abc')
sample output: { '0': 'abc',
'1': 'bca',
'2': 'cab',
'4': 'bac',
'5': 'acb',
'6': 'cba' }

Case 3)

permutationsInString('a', 'abc')
sample output: {}

My solution is Naive and Brute Force, was wondering if there are any other possible ways to reduce the time and space complexity.

function permutationsInString(longString, shortString) {
  let hash = {}; // let hash store the resulting found permutation strings and their respective index in the long string
  let permutations_shortStrings = permutationArr(shortString) ; // permute the short string and store the values in an array called permutations_shortStrings
  for(let i = 0 ; i < permutations_shortStrings.length ; i++) { // iterate through the array of permutations to check if the long string contains any of the values
    if(longString.indexOf(permutations_shortStrings[i]) !== -1)  // need to check for -1 as it indicates the substring cannot be found
      hash[longString.indexOf(permutations_shortStrings[i])] = permutations_shortStrings[i];
    }

    return hash ;

}

function permutationArr(str)
{
  var arr = str.split(''),
  permutations = [];

  function swap(a, b)
  {
    var tmp = arr[a];
    arr[a] = arr[b];
    arr[b] = tmp;
  }

// Heap's algorithm implementation
  function generate(n) {
    if (n == 1) {
      permutations.push(arr.join().replace(/,/g, ''));
    } else {
      for (var i = 0; i < n; ++i) {
        generate(n - 1);
        swap(n % 2 ? 0 : i, n - 1);
      }
    }
  }

  generate(arr.length);
  return permutations;
}

Whitespace, variable extraction, etc.

function permutationsInString(longString, shortString) {
  let hash = {}; // let hash store the resulting found permutation strings and their respective index in the long string
  let permutations_shortStrings = permutationArr(shortString) ; // permute the short string and store the values in an array called permutations_shortStrings
  for(let i = 0 ; i < permutations_shortStrings.length ; i++) { // iterate through the array of permutations to check if the long string contains any of the values
    if(longString.indexOf(permutations_shortStrings[i]) !== -1)  // need to check for -1 as it indicates the substring cannot be found
      hash[longString.indexOf(permutations_shortStrings[i])] = permutations_shortStrings[i];
    }

    return hash ;

}

What happened to the indentation? Why do some ; have a space before them and others not? Why the blank line before the closing }?

permutations_shortStrings[i] is a mouthful. Extracting a local variable with a short intelligible name would improve readability.

longString.indexOf(permutations_shortStrings[i]) is not only a bigger mouthful but also potentially very expensive. A common subexpression which takes non-constant time should definitely be extracted to a local variable.

The comments are largely noise. The only one which contributes to my understanding of the code is the one saying what hash is supposed to contain.

Bug

      permutations.push(arr.join().replace(/,/g, ''));

What output should I expect if I call permutationsArr(",;")? What do I actually get?

This bug can be fixed, and performance improved, as:

      permutations.push(arr.join(""));

Algorithm

  My solution is Naive and Brute Force, was wondering if there are any other possible ways to reduce the time and space complexity.

There most certainly are.

Step back to a simpler task: given strings a and b which are guaranteed to be of the same length, is a a permutation of b? Write a solution which doesn't calculate any permutations. Hint:

   Count the number of times each character occurs

Now apply that to your search problem. Consider longString.substring(0, shortString.length): is it a permutation of shortString? Now consider advancing one character through longString: you remove longString.charAt(i) and add longString.charAt(shortString.length + i). How can you update the data structures used to test whether it's a permutation of shortString in constant time? Hint:

   In addition to counts of the characters, you might find it useful to have a count of the number of characters for which the difference is non-zero.

More sophisticated approaches (modelled on e.g. KMP) can allow you to skip more than one character at a time, but the one I've outlined already gets you down to linear time (subject to certain assumptions) so beyond that it's just a case of improving the constant factor.


Bisection search game I'm starting to learn Python and am trying to optimize this bisection search game.

high = 100
low = 0
guess = (high + low)/2

print('Please think of a number between 0 and 100!')

guessing = True
while guessing:
    print('Is your secret number ' + str(guess) + '?')
    pointer = raw_input("Enter 'h' to indicate the guess is too high. Enter 'l'   to   indicate the guess is too low. Enter 'c' to indicate I guessed correctly.")
    if pointer == 'h':
        high = guess
        guess = (low + guess)/2

    elif pointer == 'l':
        low = guess
        guess = (high + guess)/2

    elif pointer == 'c':
        guessing = False

    else:
        print('Sorry, I did not understand your input.')
print('Game over. Your secret number was: ' + str(guess))

Some things I think would improve your code, which is quite correct:

Having variables for high and low, you shouldn't hard code their values in the opening print.
You should use // to make sure you are getting integer division.
You can write guess = (low + high) // 2 only once, if you place it as the first line inside the while loop.
When checking for pointer, you may want to first convert it to lower case, to make sure both h and H are understood.
Make your code conform to PEP8 on things like maximum line length.
Using the format method of str can make more clear what you are printing.

Putting it all together:

high, low = 100, 0

print('Please think of a number between {0} and {1}!'.format(low, high))

guessing = True
while guessing:
    guess = (low + high) // 2
    print('Is your secret number {0}?'.format(guess))
    pointer = raw_input("Enter 'h' to indicate the guess is too high. "
                        "Enter 'l' to indicate the guess is too low. "
                        "Enter 'c' to indicate I guessed correctly.").lower()
    if pointer == 'h' :
        high = guess
    elif pointer == 'l' :
        low = guess
    elif pointer == 'c':
        guessing = False
    else:
        print('Sorry, I did not understand your input.')

print('Game over. Your secret number was {0}.'.format(guess))


Producer consumer water cooler example I have written a water cooler example in which the consumer drinks from the water cooler until it's empty and when it gets empty the producer fills it up. The consumers wait until it's filled up and then start drinking again. Can you please review?

Producer

package com.par.consumer.producer;

import java.util.List;

public class Producer implements Runnable {
    private List<Integer> mySharedList;
    int size;

    public Producer(List<Integer> sharedList,int size) {
        this.mySharedList=sharedList;
        this.size=size;
    }

    @Override
    public void run() {
        while(true)
        {
            try {
                Thread.sleep(1000);

            } catch (InterruptedException e1) {
                e1.printStackTrace();
            }

            synchronized (mySharedList) {
                System.out.println("Size is "+mySharedList.size());

                if(mySharedList.isEmpty()) // Water cooler is empty fill it up
                {
                    for(int i=1;i<=size;i++){
                        System.out.println("Adding water "+i);
                        mySharedList.add(i);
                    }
                    mySharedList.notify();
                }

                try {
                    System.out.println("Producer waiting");
                    mySharedList.wait();
                    System.out.println("Producer waiting exit");
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

Consumer

package com.par.consumer.producer;

import java.util.List;

public class Consumer implements Runnable{
    private List<Integer> mySharedList;

    public Consumer(List<Integer> sharedList) {
        this.mySharedList=sharedList;
    }
    @Override
    public void run() {
        while(true)
        {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e1) {
                e1.printStackTrace();
            }
            synchronized (mySharedList) {
                if(mySharedList.isEmpty())
                {
                    System.out.println("IS EMPTY");

                    try {
                        mySharedList.notify();
                        System.out.println("Consumer waiting");
                        mySharedList.wait();
                        System.out.println("Consumer waiting exit");
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                }

                System.out.println("Drinking "+mySharedList.remove(0));
            }
        }
    }
}

WaterCooler (Driver)

package com.par.consumer.producer;

import java.util.ArrayList;
import java.util.List;

public class WaterCooler {
    volatile List<Integer> list=new ArrayList<Integer>();

    public static void main(String[] args) {
        new WaterCooler().run();
    }

    void run()
    {
        new Thread(new Producer(list, 4)).start();
        new Thread(new Consumer(list)).start();
    }
}

Producer and Consumer

You properly synchronize on the sharedList.
Both Producer and Consumer fail to use the proper wait() idiom to guard against spurious wake up.

Producer waits until the list is empty :

while(!sharedList.isEmpty())
    sharedList.wait();

Consumer waits until the list is not empty :

while(sharedList.isEmpty())
    sharedList.wait();

Given that both wait on the same monitor with a different condition you must use notifyAll() instead of notify() :

  Single notify can be used instead of notifyAllonly when both of the
  following conditions hold:

  Uniform waiters. Only one condition predicate is associated with the condition queue, and each thread executes the same logic upon
  re-turning from wait
  One-in, one-out. A notification on the condition variable enables at most one thread to proceed.

(from Java Concurrency in Practice)
You set the threads in motion, but you have no way of stopping them. Consider using interruption for this.

Check the interrupted flag in your loop condition :

while (!Thread.currentThread().isInterrupted())

Properly deal with InterruptedException by restoring the interrupted state so the interruption is not lost.

Call Thread.interrupt() in the catch block :

} catch (InterruptedException e) {
    Thread.currentThread().interrupt();
}

I assume this is some sort of exercise. Typically to connect producers and consumers you would use a blocking bounded queue.

Latstly : while println() gives you some nice output, it can give you a false sense of thread safety, as it does synchronisation under the hood, and in doing so introduces memory boundaries that may hide problems with your code, that will surface once you substitute them with actual production code.

WaterCooler

There is no reason to make list volatile. It is never reassigned. In fact it would be better to mark it final, to make sure this never happens.
Creating a thread, and starting it, gives you little control over it. (how will you stop them?)


LuhnyBin - my first Ruby script I created my first Ruby program. Even though it works, I wondered whether I could do something different/better?

#!/usr/bin/ruby

class LuhnyBinChecker
  def initialize(number)
    @number = number
  end

  def check
    createNumberArray
    doubleSecond
    sumNumbers
    getResult
  end

  def createNumberArray
    @numberArray = @number.scan(/\d/).map { |c| c.to_i }
  end

  def doubleSecond
    puts "Checker: doubling every second number from the right.."
    i = @numberArray.length - 1
    pos = 0
    while i >= 0
      if pos.modulo(2) == 1
        @numberArray[i] = @numberArray[i] * 2
      end
      i = i - 1
      pos = pos + 1
    end
  end

  def sumNumbers
    puts "Checker: calculating the sum.."
    @sum = 0
    @numberArray.each do |i|
      if i > 9
        @sum = @sum + splittedSum(i) # treat numbers >= 10 individually
                                     # e.g. 12 -> 1 + 2 = 3
      else
        @sum = @sum + i
      end
    end
  end

  def splittedSum(number)
    numberAsString = number.to_s()
    splittedNumbers = numberAsString.scan(/\d/).map { |c| c.to_i }
    splittedSum = 0
    splittedNumbers.each do |n|
      splittedSum = splittedSum + n
    end
    return splittedSum
  end

  def getResult
    if @sum.modulo(10) == 0
      puts "Credit card number is valid"
    else
      puts "Credit card number is not valid"
    end
  end
end

class CreditCard
  def setCardNumber(number)
    @cardNumber = number
  end

  def getCardNumber
    if @cardNumber!= nil
      @cardNumber
    end
  end

  def verify
    puts "Verifying card number now..."
    checker = LuhnyBinChecker.new(@cardNumber)
    checker.check
  end
end

if __FILE__ == $0
  card = CreditCard.new

  puts "Please enter the credit card number to check:"
  gets              # input card number
  input = $_
  if input.match(/[a-z]/i) != nil || input.match(/\d/) == nil
    puts "Please enter only numbers."
    exit
  end

  card.setCardNumber(input)
  puts "You entered: " + card.getCardNumber
  card.verify
end

Edit: here are two input/output examples:

michael@michael-linlap /media/Data/development/private/scripts $ ruby LuhnyBin.rb
Please enter the credit card number to check:
1111 1111 1111 1111 1111
You entered: 1111 1111 1111 1111 1111
Verifying card number now...
Checker: doubling every second number from the right..
Checker: calculating the sum..
Credit card number is valid

michael@michael-linlap /media/Data/development/private/scripts $ ruby LuhnyBin.rb
Please enter the credit card number to check:
fooobar1111
Please enter only numbers.

Welcome to ruby :), here are a few changes you might want to consider.

#!/usr/bin/ruby

class LuhnyBinChecker

  def initialize(number)
    @number = number
  end

Try to keep your methods pure without i/o, and collect the i/o in some portion that is dedicated to it. It helps later while you refactor the code. It is also nicer to not to rely on member variables too much, and preserve the referential transparency of functions where possible.

  def check
    arr = digits(@number)
    puts "Checker: doubling every second number from the right.."
    arr = doubleSecond(arr)
    puts "Checker: calculating the sum.."
    getResult(sumNumbers(arr))
  end

Always be on the lookout for things you can refactor and reuse.

  def digits(n)
    n.to_s().scan(/\d/).map{ |c| c.to_i }
  end

Two ways of doing the doubleSecond, I prefer the second way, and I think that is a more ruby way. But as a beginner, you might understand the first better. Choose which ever you like.

  def doubleSecond(arr)
    #arr.each_with_index do |item, i|
    #  arr[i] = arr[i] * 2 if (i %2 == 0)
    #end
    arr.zip((1..@number.length).to_a()).map do |e|
       e[1] * ((e[0] % 2) + 1)
    end
  end

  # treat numbers >= 10 individually
  # e.g. 12 -> 1 + 2 = 3
  def sumNumbers(arr)
    arr.map{|i|
        i > 9 ? splittedSum(i) : i
    }.inject(0,:+)
  end

You can think of Array.inject(0,:+) as Array.sum()

  def splittedSum(number)
    digits(number).inject(0,:+)
  end

  def getResult(sum)
    sum.modulo(10) == 0
  end
end

It is nicer to have a guarantee that a CreditCard will always contain a number rather than to check for nil in the getter method.

class CreditCard
  attr_reader :number
  def initialize(number)
    @number = number
  end

  def verify
    (LuhnyBinChecker.new(@number)).check
  end
end

You can use abort if some conditions fail to satisfy.

if __FILE__ == $0
  puts "Please enter the credit card number to check:"
  input = gets()              # input card number
  abort "Please enter only numbers." if input !~/(\d+\s)+/
  card = CreditCard.new(input)
  puts "You entered: " + card.number
  puts "Verifying card number now..."
  if card.verify()
     puts "Credit card number is valid"
  else
     puts "Credit card number is valid"
  end
end


Command line test runner This is a simple program to run a command-line program with given parameters and verify that it writes the expected output to stdout or stderr. The specification file is passed as the first argument. It's formatted like this:

echo foo -> foo
man -> What manual page do you want?

with program and arguments to the left of -> and output on the right.

I don't often work so low-level, so I'm most interested in whether I'm managing resources properly.

#define _GNU_SOURCE

#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define MAX_ARGS 10
#define MAX_LINE 256

const char* parse_args(char **args, const char *line)
{
    size_t index = 0;
    size_t delimiter_index = 0;
    while (args[index] != NULL && index < MAX_ARGS - 1)
    {
        ++index;
        args[index] = strtok(NULL, " ");
        if (args[index] && strncmp(args[index], "->", 2) == 0)
        {
            delimiter_index = index;
            args[index+1] = strtok(NULL, "\n");
            break;
        }
    }

    args[delimiter_index] = '\0';
    const char *expected_result = args[delimiter_index + 1];
    if (delimiter_index == 0 || expected_result == NULL)
    {
        fprintf(stderr, "Line <%s> is malformed\n", line);
        return NULL;
    }

    if (strlen(expected_result) > MAX_LINE)
    {
        fprintf(stderr,
                "Invalid specification. Only results less than %d chars supported",
                MAX_LINE);
        return NULL;
    }

    return expected_result;
}

bool run_test(char *line)
{
    char *args[MAX_ARGS] = {};
    args[0] = strtok(line, " ");
    if (!args[0]) return true;

    const char* expected_result = parse_args(args, line);
    if (!expected_result)
    {
        return false;
    }

    int stdout_pipe[2];
    if (pipe(stdout_pipe) != 0)
    {
        fprintf(stderr, "Unable to open pipe\n");
        return false;
    }

    pid_t pid = fork();
    if (pid == -1)
    {
        fprintf(stderr, "Unable to fork process\n");
        return false;
    }

    if (pid == 0)
    {
        // Redirect streams so caller can read stdout
        dup2(stdout_pipe[1], STDOUT_FILENO);
        dup2(stdout_pipe[1], STDERR_FILENO);
        close(stdout_pipe[0]);
        execvp(args[0], args);

        // exec* only ever returns if it fails
        fprintf(stderr, "Failed to execute test program: %s\n",
                strerror(errno));
        return false;
    }

    close(stdout_pipe[1]);

    char actual_result[MAX_LINE] = {};
    ssize_t bytes_read = read(stdout_pipe[0], actual_result, MAX_LINE);
    if (bytes_read == -1)
    {
        fprintf(stderr, "Unable to read program output\n");
        return false;
    }

    // Strip newline
    if (actual_result[bytes_read - 1] == '\n')
    {
        actual_result[bytes_read - 1] = '\0';
    }

    if (strcmp(actual_result, expected_result) != 0)
    {
        fprintf(stderr, "Test Failed: ");
        for (int arg = 0; arg < MAX_ARGS; ++ arg)
        {
            if (!args[arg]) break;
            fprintf(stderr, "%s ", args[arg]);
        }
        fprintf(stderr, "\n");
        fprintf(stderr, "Expected: %s\n", expected_result);
        fprintf(stderr, "Actual: %s\n", actual_result);
        return false;
    }

    return true;
}

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        fprintf(stderr, "Usage: %s <test_specification>\n", argv[0]);
        return 1;
    }

    const char *spec_pathname = argv[1];
    FILE* spec = fopen(spec_pathname, "r");
    if (!spec)
    {
        fprintf(stderr, "Cannot open <%s> for reading\n", spec_pathname);
        return 1;
    }

    int return_code = 0;

    char *line = NULL;
    size_t line_length = 0;
    while (getline(&line, &line_length, spec) != -1)
    {
        if (!run_test(line))
        {
            return_code = 1;
        }
    }

    free(line);
    return return_code;
}

fprintf(stderr, "Unable to read program output\n"); loses important information: exactly why the program output couldn't be read. Prefer perror.

Ditto for other fprintf(stderr, ....).
The parent doesn't wait for children. You may produce plenty of zombies.
The parent doesn't close(stdout_pipe[0]); when done with a child. You may run out of file descriptors.
The spec file format seems naive:

There is no way to deal with a multi-line output.
It mixes child's stdout and stderr. If the child produces both, their order is unpredictable.
It doesn't specify the child's return status.


Which way is better for db handler? Is it a bad idea for a model class hold the db handler as a property? Example code are as below. I think the second is better, but the first is not so bad in my opinion.

First:

class ActiveModel {
    protected $db;

    public function __construct() {
        $this->db = DbFactory::get('Foo');
    }

    public function save() {
        //use $this->db to do the job;
        ....
    }
}

Second:

class ActiveModel {

    public function save($db = null) {
        if ($db === null) {
            $db = DbFactory::get('Foo');
        }
        //then use $db to do the job;
        ....
    }
}

It is fine and very convenient to keep the database handler as a property.  However, you should pass it in the constructor via dependency injection, rather than call it's factory method inside the class.  This can ease unit testing when you need to mock a database connection object.

public function __construct($db) {
    $this->db = $db;
}


Cleanly setting ACLs So this is my full script that I've been working on. My plan is to Create New, Folder, Create AD Group FS-TESTSHARE-R, Create AD Group FS-TESTSHARE-RW, Apply Both groups to the new share folder, Set Full Read permissions to FS-TESTSHARE-R, Set Full Read/Rights permissions to FS-TESTSHARE-RW, and Set full access permissions to Domain Admins and local Administrators.

I'm Pretty sure I have all of the syntax correct now (thanks to some awesome assistance I've received here), but I was hoping for some clarification on setting ACLs.

At the bottom I've redone how I was attempting to get, create, and set the ACLs Thanks to @themadtechnician), but I was curious as to if I've got all syntax correct or if I should create the set for each of the $objUser instead of putting them all in parentheses (is this the cleanest way to do this)...

$Parent = read-host -prompt "Enter full parent path that will contain the new folder (ie. \\eccofs01\Groups\ECCO IT\)"
$Name = read-host -prompt "Enter New Folder Name."
$Path = "$($parent)$($Name)"
$Location = read-host -prompt "Enter the AD Security Group Location (i.e. Global, Americas, Europe, Asia Pacific)"

Import-Module ActiveDirectory

#Create Security Groups
$GroupParams1= @{
    'Name' = "FS-$NAME-RW"
    'SamAccountName' = "FS-$NAME-RW"
    'GroupCategory' = "Security"
    'GroupScope' = "Global"
    'DisplayName' = "$NAME Read-Write Access"
    'Path' = "CN=$LOCATION,CN=SECURITY GROUPS,CN=FILE SHARE GROUPS,DC=ESG,DC=INTL"
    'Description' = "Members of this group have read-write access to $Path."
}

New-ADGroup @GroupParams1

$GroupParams2= @{
    'Name' = "FS-$NAME-R"
    'SamAccountName' = "FS-$NAME-R"
    'GroupCategory' = "Security"
    'GroupScope' = "Global"
    'DisplayName' = "$NAME Read-Write Access"
    'Path' = "CN=$LOCATION,CN=SECURITY GROUPS,CN=FILE SHARE GROUPS,DC=ESG,DC=INTL"
    'Description' = "Members of this group have read access to $Path"
}

New-ADGroup @GroupParams2

# Create New Folder
New-Item -Path $Path -ItemType Directory

#Set Full Control Permissions
$Rights = [System.Security.AccessControl.FileSystemRights]"FullControl"

$InheritanceFlag = [System.Security.AccessControl.InheritanceFlags]"ObjectInherit,ContainerInherit"
$PropagationFlag = [System.Security.AccessControl.PropagationFlags]::None

$objType =[System.Security.AccessControl.AccessControlType]::Allow

$objUser = New-Object System.Security.Principal.NTAccount("Administrators","esg.intl\Domain Admins","esg.intl\FS-$NAME-RW")

$objACE = New-Object System.Security.AccessControl.FileSystemAccessRule($objUser, $Rights, $InheritanceFlag, $PropagationFlag, $objType)

$objACL = Get-ACL $Path

$objACL.AddAccessRule($objACE)

Set-ACL $Path $objACL

#Set Read and Execute Permissions
$Rights = [System.Security.AccessControl.FileSystemRights]"ReadAndExecute"

$InheritanceFlag = [System.Security.AccessControl.InheritanceFlags]"ObjectInherit,ContainerInherit"
$PropagationFlag = [System.Security.AccessControl.PropagationFlags]::None

$objType =[System.Security.AccessControl.AccessControlType]::Allow

$objUser = New-Object System.Security.Principal.NTAccount("esg.intl\FS-$NAME-R")

$objACE = New-Object System.Security.AccessControl.FileSystemAccessRule($objUser, $Rights, $InheritanceFlag, $PropagationFlag, $objType)

$objACL = Get-ACL $Path

$objACL.AddAccessRule($objACE)

Set-ACL $Path $objACL

To streamline the permission handling you may want to wrap ACE creation in a function and set the ACL just once at after all ACEs were added:

function New-Ace {
  [CmdletBinding()]
  Param(
    [Parameter(Mandatory=$true, Position=0)]
    [Security.Principal.NTAccount]$Account,
    [Parameter(Mandatory=$false, Position=1)]
    [Security.AccessControl.FileSystemRights]$Permissions = 'ReadAndExecute',
    [Parameter(Mandatory=$false, Position=2)]
    [Security.AccessControl.InheritanceFlags]$InheritanceFlags = 'ContainerInherit,ObjectInherit',
    [Parameter(Mandatory=$false, Position=3)]
    [Security.AccessControl.PropagationFlags]$PropagationFlags = 'None',
    [Parameter(Mandatory=$false, Position=4)]
    [Security.AccessControl.AccessControlType]$Type = 'Allow'
  )

  New-Object Security.AccessControl.FileSystemAccessRule(
    $Account, $Permissions, $InheritanceFlags, $PropagationFlags, $Type
  )
}

$domain = 'DOMAIN'

$acl = Get-Acl $path

'Administrators', "$domain\Domain Admins" | ForEach-Object {
  $acl.AddAccessRule((New-Ace $_ 'FullControl'))
}
$acl.AddAccessRule((New-Ace "$domain\FS-$NAME-RW" 'Modify'))
$acl.AddAccessRule((New-Ace "$domain\FS-$NAME-R" 'ReadAndExecute'))

Set-Acl $path $acl

Other improvements might include determining the administrators and domain admins group names independent of the system's locale by their well-known SIDs, e.g.:

$administrators = ([wmi]"Win32_Sid.Sid='S-1-5-32-544'").AccountName

$domain_admins  = Get-WmiObject -Class Win32_Account |
                  Where-Object { $_.SID -like '*-512' } |
                  Select-Object -Expand Caption

Another thing you could do is map names to permissions like this:

$permissions = @{
  'Administrators'        = 'FullControl'
  "$domain\Domain Admins" = 'FullControl'
  "$domain\FS-$NAME-RW"   = 'Modify'
  "$domain\FS-$NAME-R"    = 'ReadAndExecute'
}

and use the hashtable as input for the ACE creation:

$permissions.Keys | ForEach-Object {
  $acl.AddAccessRule((New-Ace $_ $permissions[$_]))
}

Of course this implies that all ACEs have the same type and flags, otherwise you'd have to use a more elaborate data structure.


Less-repetitive code for document-analyzer I've refactored my code such that every function has only one responsibility. Now I'd like like to work on making it DRY.

I'm noticing a lot of duplication in the DocumentAnalyzer class. In nearly every function, I make a reference to @document.

Should my one-statement functions be collapsed into one line?

require 'set'

class Document
    attr_reader :words
    def initialize(words)
        @words = words
    end
end

class DocumentAnalyzer
    def initialize(document)
      @document = document
    end

    def unique_words
        @document.words.to_set
    end

    def unique_words_count
        unique_words.count
    end

  def word_count
    @document.words.size
  end

  def total_characters
    total = 0.0
    @document.words.each { |word| total += word.size }
    total
  end

  def average_word_length
    total_characters / word_count
  end
end

d = Document.new %w(here are some words)
da = DocumentAnalyzer.new(d)

To my eyes, there should really only be one class: Document.

As you noticed, the methods in DocumentAnalyzer all use @document. The analyzer class is so tied up in Document (and its methods are so simple) that it probably shouldn't be a wholly separate class.

But if there's a reason for it to be separate class (I can't think of any), then there's nothing inherently wrong in referencing the same variable in several places.

DRY does not mean "Don't ever repeat anything, ever!". It means "Don't duplicate logic". Accessing a variable isn't in itself logic; logic is what you do with it.

So if you want to always upcase all the words (or something), you should make a method to do that for you, instead of repeating that bit of logic everywhere you need it. But even so, you'd still have to call your upcased_words method in all those places, so it's not like you can avoid any and all repetition.

And no, don't collapse your methods. A single line is still a method body. Collapsing code is practically only used when you make a completely empty method or class on purpose, and want to really highlight that it's intended to be empty. E.g. def no_op; end.
You'll also see it in code that defines custom exception classes. The classes just inherit from an existing exception class, but they neither add or override anything; the point is just to define them as distinct classes. Hence, they're usually defined with class CustomError < ExistingError; end

Other things I noticed:

Fix your indentation. Sometimes it's 2 spaces, sometimes it's 4. Stick to 2.
Your Document class could - in its current form - be replaced by a plain array, really. It doesn't really do anything as it is right now. I'd expect a class named Document to (for instance) have methods to load/read a file or something. Instead, you give it an array, and it stores that array. But if you already have that array, why bother?
You don't need Set. You can just do words.uniq
Your total_characters method is odd. Why does total start out as a float? It'll always be an integer (unless you've discovered fractional characters)
Speaking of, the summing up can be done with inject (aka reduce)

def total_characters
  @document.words.inject(0) { |sum, word| sum += word.length }
end

Also, the method should probably be called character_count to stick to the convention established by word_count and unique_word_count. Neither of those is called total_...
You should probably memoize your methods. That is, store the return value after calculating it the first time, so it doesn't have to be calculated again later:

def total_characters
  @total_characters ||= @document.words.inject(0) { |sum, word| sum += word.length }
end

This code will return the instance variable immediately unless it's nil or false. Otherwise, it'll actually evaluate the right-hand expression (i.e. sum up the character count), assign the result to the instance variable, and then return the value.
If you want to memoize return values, you should also freeze the words array. Otherwise you can do something like some_document.words.fill("foobar") after which your previously computed values stop making sense.
I might just skip the *_count methods, since they're not even shortcuts. Anyone can call some_document.words.count or some_document.unique_words.count. Heck, you could even skip unique_words since it's the same as words.uniq (which is even shorter!).
Basically, if the class itself doesn't rely on a method, and the logic is trivial, there's no reason to add that method (i.e. unique_words isn't used by the class, and is very trivial).
Edit As jwg points out in the comments, memoization might be a good reason to add a method "ahead of time". Still, if the class itself doesn't have an obvious use for the method, think twice before adding it.

Here's what I might write

class Document
  attr_reader :words

  def initialize(words)
    @words = words.dup.freeze
  end

  def characters
    @characters ||= words.flat_map { |word| word.chars.to_a }.freeze
  end

  def average_word_length
    return 0 if words.empty? # avoid division by zero
    characters.count / words.count.to_f
  end
end

Again, I don't think the *_count methods add anything, since you can just write .count anyway:

doc = Document.new(%w(here are some words))
doc.words               # => ["here", "are", "some", "words"]
doc.characters          # => ["h", "e", "r", "e", "a", "r", "e", "s", "o", "m", "e", "w", "o", "r", "d", "s"]
doc.average_word_length # => 4.0
doc.words.count         # => 4
doc.characters.count    # => 16
doc.words.uniq          # => ["here", "are", "some", "words"]
doc.words.uniq.count    # => 4

As you can see, I've also left out the unique_words method, because it's so simple. However, if the words array is very long, it might be more efficient to provide a memoized method for people to use. I.e.

def unique_words
  @unique_words ||= words.uniq
end


Distributed (asynchronous) averaging This code simulates a distributed averaging protocol. There is a network of nodes, and at the clicks of a Poisson process:

A random node wakes up
Transmits it's current average to all the nodes it is connected to
These nodes have a coroutine which waits to accept new values and compute the average.

from numpy.random import exponential, randint, uniform
import time
from itertools import product

class Node(object):
    def __init__(self, number):
        self.friends = []
        self.count = 0
        self.val = number
        self.average = number
        self.avg = self.averager()
        self.avg.next()

    def averager(self):
        while True:
            term = yield self.average
            self.val += term
            self.count += 1
            self.average = self.val/self.count

class Network(object):
    def __init__(self, num_nodes):
        self.nodes = [0]*num_nodes
        for i in range(num_nodes):
            self.nodes[i] = Node(randint(1, 10))
        self.set_connections()

    def set_connections(self):
        for f, g in product(self.nodes, self.nodes):
            p = uniform()
            if p < 0.8:
                f.friends.append(g)
                g.friends.append(f)

    def mainloop(self):
        num_nodes = len(self.nodes)
        while True:
            next_time = exponential(0.1)
            node = self.nodes[randint(num_nodes)]
            for friend in node.friends:
                friend.avg.send(node.average)
                print friend.average
            time.sleep(next_time)

So far I've only tested it as follows:

>>> n = Network(10)
>>> n.mainloop()

and then observed that the friend.average printed out all converge to the same value.

My main concern about this code is that a lot of the logic for waking up nodes is in the Network class, whilst I'd rather it was in the node class. Also, there's no exception/error handling, or proper testing.

The main purpose of this was to try and understand coroutines, which was a bit of a mind bender.

The way that the random graph is generated seems suspicious:

class Network(object):
    def __init__(self, num_nodes):
        self.nodes = [0]*num_nodes
        for i in range(num_nodes):
            self.nodes[i] = Node(randint(1, 10))
        self.set_connections()

    def set_connections(self):
        for f, g in product(self.nodes, self.nodes):
            p = uniform()
            if p < 0.8:
                f.friends.append(g)
                g.friends.append(f)

First of all, self.nodes should just be defined as

self.nodes = [Node(randint(1, 10)) for _ in range(num_nodes)]

The way that nodes changes type from a list of integers to a list of Node objects makes it feel slightly more dirty.

It looks like set_connections() is only meant to be called from the constructor, so it should be renamed _set_connections() to suggest that it is private.  However, considering that __init__() would just consist of two lines (self.nodes = … and self._set_connections()), I would just lump everything into __init__().

product(self.nodes, self.nodes) seems wrong.  It's possible to cause a Node to befriend itself.  It's also possible to have two nodes list each other as friends twice.  I suspect that what you want is combinations(self.nodes, 2).

def __init__(self, num_nodes):
    self.nodes = [Node(randint(1, 10)) for _ in range(num_nodes)]
    for f, g in combinations(self.nodes, 2):
        if uniform() < 0.8:
            f.friends.append(g)
            g.friends.append(f)


Monitoring real-time air waves I have a program that monitors real-time air waves and demodulates them into useable data. The program runs fine and with very little issues over time. The only thing I am really interested in is whether the calls to memcpy can be handled differently.

I have tried using valgrind and this code does not seem to be leaking memory.

static output_buffer o_buffers[MAX_OUTPUT_BUFF_COUNT]; //MAX_OUTPUT_BUFF_COUNT = 100
static int o_buffer_count = 0;

struct output_buffer
{
    int16_t buf[MAXIMUM_BUF_LENGTH]; //= 262144
    int len;
};

static void rtlsdr_data_callback(unsigned char *buf, uint32_t len, void *ctx)
{
    if (do_exit)
        return;
    rtl_dev *dev_ctx = (rtl_dev *)ctx;

    demod_state *d = &dev_ctx->demod;

    rotate_90(buf, len);

    int16_t buf16[MAXIMUM_BUF_LENGTH];

    //normalize the data
    for (int i = 0; i < (int)len; i++)
        buf16[i] = (int16_t)buf[i] - 127;

    //create a new output buffer
    output_buffer o;
    memcpy(o.buf, buf16, 2 * len);
    o.len = len;

    if (do_exit)
        return;

    //notify the demodulation thread
    pthread_rwlock_wrlock(&d->rw);
    o_buffers[o_buffer_count] = o;
    o_buffer_count++;
    pthread_rwlock_unlock(&d->rw);
}

static void *demod_thread_fn(void *arg)
{
    rtl_dev *ctx = (rtl_dev *)arg;

    demod_state *d = &ctx->demod;

    int fbuf_cnt = 0;
    float fbuf[MAX_DEMOD_BUFFERSIZE];
    int16_t *sp;

    d->lowpassed[0] = 0;

    while (!do_exit)
    {
        if (do_exit)
        {

            return 0;
        }
        if (!o_buffer_count)
        {
            continue;
        }

        //copy the buffers and reset the counter.. TODO: make quicker, watch for leaks
        pthread_rwlock_wrlock(&d->rw);
        int old_buffer_count = o_buffer_count;
        output_buffer *old_buffers = (output_buffer *)malloc(sizeof(output_buffer) * MAX_OUTPUT_BUFF_COUNT);
        memcpy(old_buffers, o_buffers, sizeof(output_buffer) * MAX_OUTPUT_BUFF_COUNT);

        o_buffer_count = 0;
        pthread_rwlock_unlock(&d->rw);

        for (int i = 0; i < old_buffer_count; i++)
        {
            output_buffer o = old_buffers[i];

            memcpy(d->lowpassed, o.buf, 2 * o.len);
            d->lp_len = o.len;
            //do work with the data...
        }
        free(old_buffers);
    }

    return 0;
}

Now the actual program is quite large so a working sample will be very hard to create.

However, some notes:

pthread_rwlock_wrlock(&d->rw); is referencing a pthread_rwlock_t rw;
rtlsdr_data_callback is called as soon as a buffer is ready. This method needs to exit as quick as possible to prevent data loss.
demod_thread_fn is not as important for performance as we are on another thread not responding to real-time airwaves however the block pthread_rwlock_wrlock(&d->rw); to pthread_rwlock_unlock(&d->rw); will consequently block the rtlsdr_data_callback
The "air-waves" are simple FLEX pager waves and transmissions may be within milliseconds.

This loop looks odd...

static void *demod_thread_fn(void *arg)
{
    while (!do_exit)
    {
        if (do_exit)
        {
            return 0;
        }

        ...
    }

    return 0;
}

Looks to me like you can get rid of the if. If do_exit can change
between the first check and the second, and that's important, then you have
a race condition -- it could change after the second check as well.  No
amount of rechecking can provide you any assurances.
Consider using a semaphore rather than busy-waiting for o_buffer_count to change.
Decide whether this is to be C or C++. :P  If it is supposed to be C++, then you should be using new and delete rather than malloc and free. If it's not, then you ought to either typedef your struct types or actually say struct output_buffer etc.


Random interval generator for a game I have been working on a few games which all have a need to create "events" randomly over time.  The intervals need to get shorter as time goes on if the event is negative or they need to get longer as time goes on if the event is positive in order to make the game harder the longer you play.

In my head, this picture describes how the intervals should change over time.  At first, the minimum and maximum possible time change linearly, but there is a limit for both of them which can not be exceeded once it is met.

Below is my implementation.  It is intended to be used by another object which will actually create the event.  The parent object should repeatedly call ShouldDoEvent with the amount of time that has passed since the last time it was called.

My concern is that the getNextInterval method only makes sense if you have the picture in your head and have worked out the algebra.  How can I make that method clearer?

using UnityEngine;
using System.Collections;

public class RandomIntervals
{
    private Vector2 _startMax;
    private Vector2 _startMin;
    private Vector2 _endMax;
    private Vector2 _endMin;

    private float _elapsedTime = 0;
    private float _nextEvent = -1f;
    private System.Random _random = new System.Random();

    public RandomIntervals(Vector2 startMax, Vector2 startMin, Vector2 endMax, Vector2 endMin)
    {
        _startMax = startMax;
        _startMin = startMin;
        _endMax = endMax;
        _endMin = endMin;
    }

    public bool ShouldDoEvent(float deltaTime)
    {
        _elapsedTime += deltaTime;

        if (_elapsedTime < _nextEvent) return false;

        _nextEvent = _elapsedTime + getNextInterval();
        return true;
    }

    private float getNextInterval()
    {
        float max, min;

        if (_elapsedTime <  _endMax.x)
        {
            float percentBetween = _elapsedTime / _endMax.x;
            var maxPoint = _startMax + percentBetween * (_endMax - _startMax);
            max = maxPoint.y;
        }
        else
        {
            max = _endMax.y;
        }

        if (_elapsedTime < _endMin.y)
        {
            float percentBetween = _elapsedTime / _endMin.x;
            var minPoint = _startMin + percentBetween * (_endMin - _startMin);
            min = minPoint.y;
        }
        else
        {
            min = _endMin.y;
        }

        return min + ((float)_random.NextDouble()) * (max - min);
    }
}

It might be worth introducing an abstraction for the type of piecewise linear functions that bound your intervals. For example (warning, there might be errors)

class PiecewiseLinear
{
    private readonly Vector2 start;
    private readonly Vector2 end;

    public PiecewiseLinear(Vector2 start, Vector2 end)
    {
        this.start = start;
        this.end = end;
    }

    public double ValueAt(double x)
    {
        if (x >= end.x)
        {
            return end.y;
        }

        return (start + (x / (end.x - start.x)) * (end - start)).y;
    }
}

The class name is not ideal, as the class only deals with a very specialised case, but I can't think of a more specific name.

Anyway, you can now use it like this

var max = new PiecewiseLinear(new Vector2(0, 5), new Vector2(3, 2));
var min = new PiecewiseLinear(new Vector2(0, 4), new Vector2(2, 1));

for (double x = 0; x <= 4; x += 0.25)
{
    Console.WriteLine("({0}, {1})", x, max.ValueAt(x) - min.ValueAt(x));
}


Triangle categorizer -- completely refactored and re-written While reviewing a C question I started writing my own implementation and in no time I had a completely refactored completely different implementation. As the asker is learning I did not want to post it, instead I ask you for further review and possibilities of improvement.

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

int get_side(name) {
    printf("Side-%d has value: ", name);
    int value; scanf("%d", &value);
    return value;
}

bool is_valid_triangle(side_1,side_2,side_3) {
    bool all_positive = side_1 > 0 && side_2 > 0 && side_3 > 0;
    bool correct_sized_sides = side_1 + side_2 > side_3 && \
                               side_2 + side_3 > side_1 && \
                               side_3 + side_1 > side_2;
    return all_positive && correct_sized_sides;
}

bool is_equilater(side_1,side_2,side_3) {
    return (side_1 == side_2 && side_2 == side_3);
}

bool is_isosceles(side_1,side_2,side_3) {
    return (side_1 == side_2) || (side_2 == side_3) || (side_3 == side_1);
}

bool is_scalen(side_1,side_2,side_3) {
    return ! is_isosceles(side_1,side_2,side_3);
}

const char * triangle_categorizer(side_1,side_2,side_3) {
    if (is_equilater(side_1,side_2,side_3)) {
        return "equilater";
    }

    if (is_isosceles(side_1,side_2,side_3)) {
        return "isosceles";
    }

    if (is_scalen(side_1,side_2,side_3)) {
        return "scalen";
    }
}

int main() {
    int side_1 = get_side(1);
    int side_2 = get_side(2);
    int side_3 = get_side(3);

    if (! is_valid_triangle(side_1,side_2,side_3)) {
        puts("The triangle you intered is invalid!");
        return 1;
    }
    printf("The triangle you entered is %s.\n", \
           triangle_categorizer(side_1,side_2,side_3));
    return 0;
}

A few simple comments:

Spelling:

Triangles are either Isosceles, Equilateral, or Scalene

Also instead of using string literals, replace them with macros like so:

#define EQUILATERAL "equilateral"
#define ISOSCELES "isosceles"
#define SCALENE "scalene"

I commend you on using long names in c, as many c-programmers use 1-2 letter names for things. I might suggest adding comments to complex code, such as mentioning the fact that you are using triangle inequality in your is_valid_triangle function

I would also suggest extending this program to work with non-integers, such as long longs, doubles and the like. In addition, it may be more helpful to a user to pass in coordinates instead of side lengths. That would also allow you to not check if a triangle is valid or not, as technically all 3 point pairs form a triangle (even if it is degenerate).


PHP script that returns an error message based on what $_POST parameters are missing I have a script that checks if the specified set of $_POST variables are supplied and not empty and returns an error string that is human readable.
Here's my code:
if ($_SERVER["REQUEST_METHOD"] === "POST") {
    // Local variables initialization
    $missing = [];

    $checkVariables = [
        'name', 'address', 'contact'
    ];

    // Check for nulls or non existent payload
    foreach ($checkVariables as $item) {
        if (empty($_POST[$item]) || !array_key_exists($item, $_POST)) {
            $missing[] = $item;
        }
    }

    $missingCount = count($missing);
    if ($missingCount > 0) {
        $missingString = '';

        if ($missingCount === 1) {
            $missingString = $missing[0];
        } else if ($missingCount === 2) {
            $missingString = implode(' and ', $missing);
        } else {
            $lastItem = array_pop($missing);
            $partialMissingString = implode(', ', $missing);

            $missingString = $partialMissingString . ", and {$lastItem}";
        }

        $verb = $missingCount > 1 ? 'are' : 'is';
        $field = $missingCount > 1 ? 'fields' : 'field';
        $errorString = "Incomplete information received. " .
            "The {$missingString} {$field} {$verb} missing. " .
            "Please check your inputs and try again.";

        // Exits the script
        OutputResponse::sendResponse(400, $errorString);
    }
    // if there's no missing continue running the script
    // more code here but it's not the focus of this code review
}

My OutputResponse code:
class OutputResponse
{
    public static function sendResponse(string $statusCode, $messageBody): void
    {
        if ($statusCode == 500) {
            header("HTTP/1.1 500 Internal Server Error");
        } else if ($statusCode == 405) {
            header('HTTP/1.1 405 Method Not Allowed');
        } else if ($statusCode == 404) {
            header("HTTP/1.1 404 Not Found");
        } else if ($statusCode == 403) {
            header("HTTP/1.1 403 Forbidden");
        } else if ($statusCode == 401) {
            header("HTTP/1.1 401 Unauthorized");
        } else if ($statusCode == 400) {
            header("HTTP/1.1 400 Bad Request");
        } else {
            header("HTTP/1.1 200 OK");
        }

        header('Content-Type: application/json');
        echo json_encode([
            'code' => $statusCode,
            'data' => $messageBody
        ]);

        exit;
    }
}

The script expects $_POST['name'], $_POST['address'], and $_POST['contact'] to be not null or empty. The array keys to be checked are in the $checkVariables array.
For simplification, I'm dropping the $_POST in the next examples.
If name is the only one missing:
{
    "code": "400",
    "data": "Incomplete information received. The name field is missing. Please check your inputs and try again."
}

If name and contact are missing:
{
    "code": "400",
    "data": "Incomplete information received. The name and contact fields are missing. Please check your inputs and try again."
}

If all three are missing:
{
    "code": "400",
    "data": "Incomplete information received. The name, address, and contact fields are missing. Please check your inputs and try again."
}

Personally, the part on the if ($missingCount > 0) block is the messiest or unoptimized.
Any improvements or critiques are welcome!

if (empty($_POST[$item]) || !array_key_exists($item, $_POST)) { is literally checking if $_POST[$item] is not declared or is falsey THEN checking if $_POST[$item] is not declared.

// Check for nulls or non existent payload

The comment associated with the step indicates that you want to check if $_POST elements are not declared or are null.  In this case, you should only use: if (!isset($_POST[$item])) {.
If you want to check that elements are not declared or empty/falsey, then use:if (empty($_POST[$item])) {.

if ($missingCount > 0) { can be safely reduced to if ($missingCount) { which checks if the count is "truthy" (in other words, not zero).

else if is one word in php, please always type elseif to comply with PSR guidelines.

Don't bother declaring $missingString = ''; if you are unconditionally going to overwrite it anyhow.

You can combine the if and first else if because if the array only has one element, then no glue will be used while producing the output string.
if ($missingCount < 3) {
     $missingString = implode(' and ', $missing);
}

The "smart-implode" as I've seen it referred to at least once on StackOverflow is fine enough.  There are a few other ways to do it -- I'll show another one below.

The whole chunk of code that crafts the OutputResponse might look like this: (Demo).  The advantages are: fewest declared variables (specifically no single-use variables), not repeated function calls, and the elegance of sprintf() when injecting variables into a string.
$missingCount = count($missing);
if ($missingCount) {
    if ($missingCount < 3) {
         $glue = ' and ';
    } else {
         $glue = ', ';
         $missing[] = 'and ' . array_pop($missing);
    }
    OutputResponse::sendResponse(
        400,
        sprintf(
            'Incomplete information received. The %s %s missing. Please check your inputs and try again.',
            implode($glue, $missing),
            $missingCount === 1 ? 'field is' : 'fields are'
        )
    );
}

[EDITED] After reading the other reviews and learning that my recommended lookup array was incomplete, flawed, and frankly suboptimal, I now find myself agreeing with YourCommonSense's advice to use http_response_code() for setting the HTTP response code.  See this Stack Overflow page for additional considerations and fringe cases around the native function which was available since PHP5.4. Also,  Now I suppose my new code for the sendResponse() method would look like this:
 http_response_code($statusCode);
 header('Content-Type: application/json');
 exit(
     json_encode([
         'code' => $statusCode,
         'data' => $messageBody
     ])
 );