Best way to replace Urls in .Net application I have an application developed in ASP.NET Core C#, where according to a configuration it must replace certain urls of a text when an http request is made and the text must be returned as soon as possible in response, the text can change on each request.

At this moment the code I have is working well, but I would like you to help me check the code to improve the speed of replacement of the urls.

string r = contenidoRespuesta; //contenidoRespuesta can be html, json or javascript content
string dominio="example.com";
int puertoHttp=81,puertoHttps=444;
foreach (var p in grupo.Paginas.Where(p => p.Proxy))
{

    Uri uri = new Uri(p.URL);
    string pattern1 = string.Format("(?<=https://){0}(:(443)(?!\\d+))?(?!:)", uri.Host);//generates a regular expression to replace urls that start with "https"
    string pattern2 = string.Format("(?<=http://){0}(:(80)(?!\\d+))?(?!:)", uri.Host);//generates a regular expression to replace urls that start with "http"
    string pattern3 = string.Format("(?<!%2F|\\.|http(s)?(://)){0}(:((80)|(443))(?!\\d+))?(?!:)", uri.Host);//generates a regular expression to replace urls that start with "//"

    r = Regex.Replace(r, pattern1,uri.Host.Replace(".", string.Empty) + "." +dominio + ":" + puertoHttps, RegexOptions.IgnoreCase);// replace https://www.page.com for https://wwwpagecom.example.com
    r = Regex.Replace(r, pattern2,uri.Host.Replace(".", string.Empty) + "." + dominio + ":" + puertoHttp, RegexOptions.IgnoreCase);// replace http://www.page.com for http://wwwpagecom.example.com
    r = Regex.Replace(r, pattern3, uri.Host.Replace(".", string.Empty) + "." + dominio + ":" +(esquema == "https"? puertoHttps.ToString(): puertoHttp.ToString()));// replace //www.page.com for https://wwwpagecom.example.com
}
return r;//return the responsd with new urls

Since you just getting the host url, remove the dots, then append it as a subdomain to the given domain with the approprate protocols (Https or Http). You can do this :

public IEnumerable<string> GetRoutedUrls(IEnumerable<string> urls, string domain, int httpPort, int httpsPort)
{
    if(urls == null) { throw new ArgumentNullException(nameof(urls)); }

    if (string.IsNullOrEmpty(domain)) { throw new ArgumentNullException(nameof(domain)); }

    foreach (var url in urls)
    {
        if (string.IsNullOrEmpty(url)) { yield return null; }

       if(Uri.TryCreate(url, UriKind.RelativeOrAbsolute, out Uri uri))
        {
            yield return uri.Scheme == "https" ? $"https://{uri.Host.Replace(".", string.Empty)}.{domain}:{httpsPort}" : $"http://{uri.Host.Replace(".", string.Empty)}.{domain}:{httpPort}";
        }
    }
}

usage :

var urls = GetRoutedUrls(grupo.Paginas.Where(p => p.Proxy).Select(x=> x.URL), "example.com", 81, 444)

UPDATE

After clarifying things, I think you don't need Regex since you're replacing an exact match of urls. Regex would be more useful if you want to search for unknown inputs.

To give you an example of that, if we want to get every single url in the response we can use

Regex.Matches(response, @"(https?://)(.*)(\d)", RegexOptions.IgnoreCase)

This would get every single url that has specified a port (e.g. https://example.com:443). Then, we can do stuff on them.

In your method, is different a bit. You already have defined the urls, and you want to check for these urls in your response, if they exist, you want to change them by appending them as subdomain to the given domain, and giving them a new port as well.

Unless if there is any special cases that is not covered or mentioned in your post (which forced you to use Regex in first place), I think it would be a straight forward approach if we just use string.Replace directly.

public string ReplaceResponseUrls(IEnumerable<string> urls, string response, string domain, int httpPort, int httpsPort)
{
    if (urls == null) { throw new ArgumentNullException(nameof(urls)); }

    if (string.IsNullOrEmpty(response)) { throw new ArgumentNullException(nameof(response)); }

    if (string.IsNullOrEmpty(domain)) { throw new ArgumentNullException(nameof(domain)); }

    var responseStringBuilder = new StringBuilder(response);

    foreach (var url in urls)
    {
        if (string.IsNullOrEmpty(url)) { continue; } // skip to the next url

        if (Uri.TryCreate(url, UriKind.RelativeOrAbsolute, out Uri uri))
        {
            responseStringBuilder.Replace(
                $"{uri.Scheme}://{uri.Host}:{uri.Port}", // old url
                $"{uri.Scheme}://{uri.Host.Replace(".", string.Empty)}.{domain}:{(uri.Scheme == "https" ? httpsPort : httpPort)}" // new url
                );
        }
    }

    return responseStringBuilder.ToString();
}

Try this approach first, do some tests on it, and let me know if you need any help with it.


Making an HTTPS request more readable I think the snippet is too nested having too many blocks. It needs to be refactored to make the logic more readable.

The try exception is the first level block
Timeout::timeout is the next level block
Some loop-like blocks inside it

How can I make it more readable?

  def https_request(server_url, header, content)
    begin
      Timeout::timeout(@@TIMEOUT_PERIOD) do
        hack_ssl_options
        uri = URI.parse(server_url)
        https = Net::HTTP.new(uri.host,uri.port)
        https.use_ssl = true
        options_mask = OpenSSL::SSL::OP_NO_SSLv2 + OpenSSL::SSL::OP_NO_SSLv3 + OpenSSL::SSL::OP_NO_COMPRESSION
        https.ssl_options = options_mask
        https.verify_mode = OpenSSL::SSL::VERIFY_NONE
        req = Net::HTTP::Post.new(uri.path, initheader = header)
        req.body = content
        headers = req.each_header.inject({}) { |h,(key, value)| h[key] = value ; h }
        resp = https.request(req)
        response_headers = resp.each_header.inject({}) { |h,(key, value)| h[key] = value ; h }
        return { code: resp.code,
                 header: response_headers,
                 body: resp.body }
      end
    rescue Timeout::Error
      return { code: @@timeout_error_code,
               header: nil,
               body: @@timeout_error_msg }
    rescue Exception => e
      return { code: @@unknown_error_code,
               header: nil,
               body: @@unknown_error_msg }
    end
  end

to answer your questions first:

you don't need begin/end if it covers whole method, you can rescue directly from method,
Net::HTTP already handles timeout, no need to build custom wrapper around it,

now there are few other things that should be addressed:

inline things when possible
don't use @@ ever, use constants if you need to
don't rescue from Exception, rescue from StandardError instead (or nothing as ruby defaults to StandardError anyways)
try to be consistent with returned result, i.e. header should always return hash, never nil. Otherwise you will need to add conditions in places where you handle code
OpenSSL::SSL::VERIFY_NONE this is bad, you want to verify certificate.
do not use Net::HTTP, there are much more rubout gems available, i.e. Typhoeus

Finally it could be rewritten to something like this:

def https_request(server_url, header, content)
  hack_ssl_options
  uri = URI.parse(server_url)

  connection = Net::HTTP.new(uri.host, uri.port)
  connection.use_ssl = true
  connection.ssl_options = OpenSSL::SSL::OP_NO_SSLv2 + OpenSSL::SSL::OP_NO_SSLv3 + OpenSSL::SSL::OP_NO_COMPRESSION
  connection.verify_mode = OpenSSL::SSL::VERIFY_NONE
  connection.read_timeout = TIMEOUT_PERIOD

  request = Net::HTTP::Post.new(uri.path, initheader = header)
  request.body = content

  response = connection.request(req)

  { code: response.code, header: response.to_hash, body: response.body }
rescue Net::ReadTimeout => e
  { code: CODE_TIMEOUT, header: {}, body: e.message }
rescue => e
  { code: CODE_UNKNOWN, header: {}, body: e.message }
end

This code works slightly different from your original version, you may need to adjust it.


Scraping and using JSON data from thousands of files in a directory I have a few thousand json files containing order histories sitting in a directory from one of our systems. Since customers can opt in to receive marketing I'm opening each json file and then processing the data to scrape our active subscribers. At a later date I plan to expand this to bring this data into our CRM.

As I'm not a Python programmer by trade so I'm wondering what I could do better to make this more functional and (for lack of a better term) 'pythonic'?

import sys, os, json
import pandas as pd

print("Scraping E-mail Subscribers")
print("Using Python version %s.%s.%s" % sys.version_info[:3])

json_path = 'D:/dev/order_archive/'
json_files = [pos_json for pos_json in os.listdir(json_path) if pos_json.endswith('.json')]

records = 0
opted_records = 0
unopted_records = 0

for index, js in enumerate(json_files):
    with open(os.path.join(json_path, js)) as json_file:
        records += 1
        json_text = json.load(json_file)
        first_name = json_text['details']['firstName']
        last_name = json_text['details']['surname']
        newsletter_opt_in = json_text['details']['questions']['newsletterOptIn']
        email = json_text['details']['email']

        if(newsletter_opt_in == 'true'):
            print('Name: %s\nE-mail: %s\n\n' % (first_name + ' ' + last_name, email))
            opted_records += 1
        elif(newsletter_opt_in != 'false'):
            unopted_records += 1

print('Processed %d records. %d opted to receive e-mail marketing. %d didn''t specify an option.' % (records, opted_records, unopted_records))

records seems to just count each enumeration of json_files. Just use len(json_files) instead as that returns the length of the json_files list, which is basically what records is.

While we're at it, index is never used, so why enumerate? Just use for js in files.

json_text is a misleading name, because that's actually a dictionary of json data! Name it data or json_data for clarity.

You call on multiple keys any one of which could raise a KeyError if the key doesn't exist or is spelled wrong. You might be very confident in the structure being consistent, but I think it's better to be safe, after all who knows what'd mess with your data or break the structure. There's two ways to ensure that you wont get errors, and I think you'd do well to use both of them.

The first way is to use try except KeyError. This is Python's error catching and it works so well that it's considered more Pythonic than testing if the key is there. However it would stop you from reading other keys if you caught that 'firstName' didn't exist, so I suggest just using this to access json_data['details']. This would also give you a shortcut, rather than needing to type that out fully for each key. Here's how it'd look:

    json_text = json.load(json_file)
    try:
        details = json_text['details']
    except KeyError:
        records -= 1
        print("Bad file {}".format(json_file))
        continue

If a KeyError arises, I'm doing a few things though what you want to do may vary. First, I subtracted one from the records count since we don't have a valid record. You might want to instead track an invalid records number, or not touch that number at all. I also print a message warning the user that one of the files had a problem in it, so they can inspect why it didn't work. You could adjust that message, adding detail about what key was missing would be good but I definitely recommend including the actual name so that they can find the specific file. Lastly I call continue, which will basically go onto the next iteration of your loop. Meaning it immediately skips to the next file, allowing your script to continue.

Now the second method you can use is called get. It's a dictionary method that allows you to specify a default value if the key doesn't exist. I think it would be better here because even if a key is missing you still want to be able to build your string at the end to print. ie. Even if 'surname' isn't included, you could just set it as a blank string and print the rest of the information. So here's how it'd look:

    first_name = details.get('firstName', '')
    last_name = details.get('surname', '')

The first parameter to get is the key name, and the second is your default value. Empty strings seem appropriate, unless you want to highlight that there's missing data by putting in values like "SURNAME" or "VOID". When you need to get a nested value like ['questions']['newsletterOptIn'] you could call get, pass an empty dictionary as default and then call get on that. Basically just chaining get calls will do what you need:

    newsletter_opt_in = details.get('questions', {}).get('newsletterOptIn', '')
    email = details.get('email', '')

Now, when I see your if command I realise that we might not even need most of these keys! So you should reorder the commands so that you just first get newsletter_opt_in and then test that to determine if you even need the other keys:

    newsletter_opt_in = details.get('questions', {}).get('newsletterOptIn', '')
    if(newsletter_opt_in == 'true'):
        first_name = details.get('firstName', '')
        last_name = details.get('surname', '')
        email = details.get('email', '')
        print('Name: {}\nE-mail: {}\n\n'.format(first_name + ' ' + last_name, email))
        opted_records += 1
    elif(newsletter_opt_in != 'false'):
        unopted_records += 1

Also you don't need brackets around the expressions for if statements. if newsletter_opt_in == 'true' is perfectly fine. Also it would be good to add a comment clarifying newsletter_opt_in != 'false'. It took me a minute to realise that it was !=, meaning that you're testing for any value that doesn't match the two boolean possibilities. It's a little confusing but a comment would clear it up easily. Though I recommend switching to actual boolean values in the data if possible. If your source can store newsletter_opt_in as a boolean then Python can read it as such.

Style notes

Instead of putting all your imports on one line you should space them out. Python likes to give each import it's own line and to group imports together. Also since you only need sys for version_info you could use from sys import version_info. That will only import the version_info tuple and makes it clearer why you need that import.

import os
import json

import pandas as pd

from sys import version_info

Using % to format strings is the old way, Python has a better way known as the str.format method. Since you're passing three values out of a tuple, you will need the unpacking operator to make it work. The unpacking operator basically takes values out of a collection (ie. a list or a tuple) and passes them as arguments. So instead of passing a tuple to format we can pass the string elements it contains. To use it, just add the * character before a collection variable.

print("Scraping E-mail Subscribers")
print("Using Python version {}.{}.{}".format(*version_info[:3])

You're using snake_case to name things, which is great! That's exactly what the Python style guide recommends. However since json_path seems to be a constant, you should use UPPER_SNAKE_CASE for it, ie. JSON_PATH.

Speaking of names, pos_json is not great. I assume it's meant to be 'possible json' but that doesn't read well. It's better to name it after what it is, a file. You can't use file as that will shadow a builtin function, but you could use f or filename. Your endswith('.json') makes it pretty clear that it's about detecting if it's a json file so you can use a more direct name that describes what these filenames actually are. Also in your loop, I don't like js for the same reason, and I'd likewise call that filename.

You can also assign multiple values at once in Python, so all your records can be set as 0 simultaneously.

records = opted_records = unopted_records = 0


Three versions of a counting-up timer in JavaScript I made 3 versions of a counting-up timer in JavaScript:

Version #1

All units of time (seconds, minutes, hours, and days) are updated every second, whether they change or not.

Pro: Shorter code.

Con: Worse performance than version #2.

const timeIntervals = [
    ["day", 86400],
    ["hour", 3600],
    ["minute", 60],
    ["second", 1]
];
var counter = 1;
var tick = () => {
    var gucci = counter;
    for (unit of timeIntervals) {
        $("#" + unit[0]).html(Math.floor(gucci / unit[1]));
        gucci %= unit[1];
    }
    counter++;
};
var timer = window.setInterval(tick, 1000);

Version #2

Each unit of time (seconds, minutes, hours, and days) is updated only at fixed intervals when they change.

Pro: Better performance than version #1.

Con: Longer code.

const timeIntervals = [
    ["day", 86400],
    ["hour", 3600],
    ["minute", 60],
    ["second", 1]
];
var counter = 1;
var tick = () => {
    let zero = false;
    for (unit of timeIntervals) {
        let element = $("#" + unit[0]);
        if (zero) {
            element.html("0");
        }
        else if (counter % unit[1] === 0) {
            element.html(
                parseInt(element.html()) + 1
            );
            zero = true;
        }
    }
    counter++;
};
var timer = window.setInterval(tick, 1000);

Version #3

Each unit of time (seconds, minutes, hours, and days) is incremented up by 1 when the counter before it reaches its maximum limit.

Pro: Doesn't require a counter variable.

Con #1: It will break if the user clicks "Inspect Element" and messes with the HTML.

Con #2: Long code.

const timeIntervals = [
    ["second", 60],
    ["minute", 60],
    ["hour", 24],
    ["day", Infinity]
];
var tick = () => {
    $("#second").html(
        parseInt($("#second").html()) + 1
    );
    for (let i = 0; i < timeIntervals.length; i++) {
        let currentElement = $("#" + timeIntervals[i][0]);
        if (parseInt(currentElement.html()) >= timeIntervals[i][1]) {
            currentElement.html("0");
            let nextElement = $("#" + timeIntervals[i + 1][0]);
            nextElement.html(parseInt(nextElement.html()) + 1);
        }
    }
};
var timer = window.setInterval(tick, 1000);

Which of these 3 codes has the best performance, readability, and structure? Do any of them have security vulnerabilities?

Performance is not a significant concern, since all three callbacks have very little code, and they execute only once per second.  Rather, you should aim for clarity.  In my opinion, Version 1 is simplest and easiest to follow.

All three techniques suffer from the same weakness with window.setInterval(…, delay):

  delay

  The time, in milliseconds (thousandths of a second), the timer should delay in between executions of the specified function or code. If this parameter is less than 10, a value of 10 is used. Note that the actual delay may be longer; see Reasons for delays longer than specified in WindowOrWorkerGlobalScope.setTimeout() for examples.

In particular, the interval may be throttled to 10 seconds for long-running scripts in background tabs, or the callback may be delayed if the JavaScript engine is busy executing other tasks.  Furthermore, the whole machine might go to sleep.

Rather, you should check the time difference with every tick.  (As a side benefit, this should help address your concern about casual Web Inspector tampering.)

Use destructuring assignment to write more meaningful names than unit[0] and unit[1].   Note that in for (unit of timeIntervals), you neglected to localize unit in any way, so it's global.

The tick function should probably be declared as const rather than var.  I'd also prefer to use let rather than var, as better software engineering practice.

Also, as better practice, use jquery.text() rather than .html(), if you know that the content is text without HTML markup.

const timeIntervals = [
    ["day", 86400000],  // milliseconds
    ["hour", 3600000],
    ["minute", 60000],
    ["second", 1000]
];
const tick = (start) => () => {
    let elapsed = Date.now() - start;
    for (let [unit, ms] of timeIntervals) {
        $("#" + unit).text(Math.floor(elapsed / ms));
        elapsed %= ms;
    }
};
let timer = window.setInterval(tick(Date.now()), 1000);

  0 days,
  0 hours,
  0 minutes,
  0 seconds


When a constructor calls a helper function, should the helper mutate the object or return part of the object? I have a private method that is called from the constructor to instantiate a bunch of objects.

I can do it two ways.

    private Monster[,] createMonsters()
    {
        Monster[,] myMonsterArray = new Monster[8, 6];

        for (int i = 0; i < 8; i++)
        {
            myMonsterArray[0, i] = new Monster(SpriteFactory.getMonster1Sprite());
            myMonsterArray[1, i] = new Monster(SpriteFactory.getMonster2Sprite());
            myMonsterArray[2, i] = new Monster(SpriteFactory.getMonster3Sprite());
            myMonsterArray[3, i] = new Monster(SpriteFactory.getMonster1Sprite());
            myMonsterArray[4, i] = new Monster(SpriteFactory.getMonster2Sprite());
            myMonsterArray[5, i] = new Monster(SpriteFactory.getMonster3Sprite());
        }

        return myMonsterArray;
    }

called with

monsterArray = createMonsters();

or

    private void createMonsters()
    {
        monsterArray = new Monster[8, 6];

        for (int i = 0; i < 8; i++)
        {
            monsterArray[0, i] = new Monster(SpriteFactory.getMonster1Sprite());
            monsterArray[1, i] = new Monster(SpriteFactory.getMonster2Sprite());
            monsterArray[2, i] = new Monster(SpriteFactory.getMonster3Sprite());
            monsterArray[3, i] = new Monster(SpriteFactory.getMonster1Sprite());
            monsterArray[4, i] = new Monster(SpriteFactory.getMonster2Sprite());
            monsterArray[5, i] = new Monster(SpriteFactory.getMonster3Sprite());
        }

    }

called with

createMonsters();

Is either better or worse?

As far as I see no-one has mentioned temporal coupling yet, so here it is.

The second one (which returns void) has this code smell. Consider the following constructor code:

createMonsters();
...
doSomethingWithMonsters();

A maintainer easily could mix up the order of the called methods which breaks your class and might turn out only at runtime. It's not possible the following:

Monster[,] monsters = createMonsters();
...
doSomethingWithMonsters(monsters);

Changing the order of the two methods simply would not compile.

Another advantage of the first one is that your field could be final (or readonly, I'm not too familiar with C# but I guess it has something like that). The following constructor code also does not compile in Java if the monsters field is final:

doSomethingWithMonsters(monsters);
monsters = createMonsters();

Furthermore, having the array return value makes the code easier to read. It's obvious from the signature what the method does, readers don't have to check the method body to figure out its side-effects. Finally, you could call createMonsters() from other methods without side-effects.

Anyway, having the separate factory class (as a constructor parameter) looks even better, it's less coupled, +1 to Simon.

See also: Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302


Is the following divide and conquer recursive algorithm for the exponentiation more efficient than the iterative one for large numbers? I have the two following algorithms. My analysis says that both of them are \$\mathcal O(m^24^n)\$ i.e they are equivalent for big numbers (more than 32 bits). Is this right? Note that m and n are the bits numbers for x and y
def pow1(x, y):
    if y == 0:
        return 1
    temp = x
    while y > 1:
        y -= 1
        temp *= x
    return temp

def pow2(x, y):
    if y == 0:
        return 1
    temp = pow2(x, y//2)
    if y & 1:    return temp * temp * x
    return temp * temp

The complexity of the first
y-1 iterations and in each iteration, a subtraction taking \$\mathcal O (\lg (y-i))\$, and a multiplication taking \$\mathcal O (im\cdot m)\$, hence the total work takes
\$T=\mathcal O(\sum\limits_{i=1}^ym^2i)=\mathcal O\left(m^2\frac{y(y+1)}2\right)=\mathcal O\left(m^24^n\right)\$
Of the second
We have \$n=\lg y\$ calls and for each, we have multiplications taking \$\mathcal O (2^im\cdot 2^im)\$, hence the total work takes
\$ T=\mathcal O\left(\sum\limits_{i=1}^{n}\left(4^im^2\right)\right)=\mathcal O\left({4^n}{m^2}\right)\tag{for large $n$}\$

I think your analysis is correct if multiplication of k-digit and ℓ-digit numbers takes Θ(kℓ) time, but more efficient multiplication algorithms are known. The most efficient algorithm in wide use (implemented in GMP) is Schönhage-Strassen, which is O(k log k log log k) for numbers of equal length. I don't know the complexity for unequal lengths, but I suspect it's O(ℓ log k log log k) for k < ℓ. Using that algorithm, or any sub-kℓ algorithm, you should find that the divide-and-conquer approach is faster.


Testing the process of assigning offers to a customer I asked a similar question yesterday, which was criticised because I posted the test without the supporting code.  Therefore I deleted the question yesterday as it was not clear enough.  I am now going to post it again with the supporting code. Please see the test code below:

[TestFixture]
public class CustomerTest
{
    private IList<Product> _products;
    private Concor _concor;
    private Chestnut _chestnut;

    private IOfferCalculator _offerCalculator;

    [OneTimeSetUp]
    public void OneTimeSetUp()
    {
        _offerCalculator = new OfferCalculator();
    }

    [SetUp]
    public void Setup()
    {
        _concor = new Concor();
        _chestnut = new Chestnut();
        _products = new List<Product>();
        _products.Add(_concor);
        _products.Add(_chestnut);
    }

    [Test]
    public void Should_receive_a_for_concor()
    {
        var expectedConcor = new List<Product>();
        expectedConcor.Add(_concor);
        var gender = "F";
        var expenditure = 1500M;
        var Customer = new Customer(gender,expenditure);
        //Act
        Customer.AssignOffers(_offerCalculator, _products);
        //Assert
        CollectionAssert.AreEqual(Customer._assignedProducts, expectedConcor);
    }
}

Here is the supporting code:

public class Concor : Product
    {
        public override bool Check(Customer customer)
        {
            if (customer._expenditure > 100 && customer._gender == "F")
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public class Chestnut : Product
    {
        public override bool Check(Customer customer)
        {
            if (customer._expenditure >  100 && customer._gender =="M")
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public abstract class Product
    {
        public abstract bool Check(Customer customer);
    }

    public interface IOfferCalculator
    {
        IEnumerable<Product> Calculate(Customer customer, IList<Product> products);
    }

    public class Customer
    {
        public string _gender { get; protected set; }
        public decimal _expenditure { get; protected set; }
        public IList<Product> _assignedProducts = new List<Product>();

        public Customer(string gender, decimal expenditure)
        {
            _gender = gender;
            _expenditure = expenditure;
        }

        public void AssignOffers(IOfferCalculator offerCalculator, IList<Product> products)
        {
            foreach (var product in offerCalculator.Calculate(this, products))
            {
                if (!_assignedProducts.Contains(product))
                {
                    _assignedProducts.Add(product);
                }
            }
        }
    }

    public class OfferCalculator : IOfferCalculator
    {
        public IEnumerable<Product> Calculate(Customer customer, IList<Product> products)
        {
            foreach (var product in products)
            {
                if (product.Check(customer))
                {
                    yield return product;
                }
            }
        }
    }

I would like some feedback about the quality of the test.  The company like to use BDD, however I am conscious that I asserting state rather than behaviour (arguing that I am testing calculations rather than orchestrations).  I would also like some feedback about my use of the: OneTimeSetup attribute and Setup attribute.  Also How would I test for a Chestnut? Add a new method or use an attribute for the test cases?

You are asking about your tests but there are issues in your code that should be addressed before you start testing.

public abstract class Product
{
    public abstract bool Check(Customer customer);
}

You made Product an abstract class but this is not necessary. It does not provide any default implementation for anything so an simple interface would be enough.

There is however one more issue with it. The Check method. It doesn't say anything about what is being checked.

customer._expenditure >  100 && customer._gender =="M"

Its implementation evaluates the above condition but there is no information about it in the method name so nobody could tell what it checks.

On top of it it even uses the hardcoded magic-number 100. This should be a constant.

You also put it inside an if/else which isn't necessary. You can just return the result.

public void AssignOffers(IOfferCalculator offerCalculator, IList<Product> products)
{
    foreach (var product in offerCalculator.Calculate(this, products))
    {
        if (!_assignedProducts.Contains(product))
        {
            _assignedProducts.Add(product);
        }
    }
}

This is the method you test but it's difficult to test it because it's doing to much: it filters the product list, it checks whether it's assigned and it adds it to another list. To me these are three different responsibilities. You should at least pass an already filtered product list.

The method also does not inform me that anything will be filtered at all. It says AssignOffers but instead of assing offers it calculates something and it works with producs that it adds to another list but it does not assign anything. Everything is misleading about this method and it's full of surprises.

Having said that I find all your tests are meaningless because the implementation is meaningless too. The code is confusing and doing completely different things that the names would suggest. Writing tests for such code is pointless.

You should first fix the main code so that it's clear what it does so that you know what you test. Try to pick names that clearly communicate the purpose and do exacly only that.

For exmaple if you Calculate something then let this method return a result of this calculation based on as few parameters as possible. Passing entire objects where you only need two properties is a code smell.

public IEnumerable<Product> Calculate(Customer customer, IList<Product> products)
{
    foreach (var product in products)
    {
        if (product.Check(customer))
        {
            yield return product;
        }
    }
}

But wait a minute! Let's take a look at what Calculate actually does... to my (not) surprise it does not calculate anything but instead it checks products against a customer!!! How confusing is that?

This is such a mess that you can throw all these tests away and first rewrite the code completely then write new tests.


Is my implementation of insertion sort is correct? I'm currently learning about different sorting algorithms and after reading on the concept of the insertion sort of how it's done I've tried to implement it by myself before seeing how it was implemented in the source from which I'm learning.

This was my implementation:

void insertionSort(int arr[])
{
    for(int i = 1; i < SIZE; i++)
    {
        int j = i;
        while(j > 0 && arr[j] < arr[j - 1])
        {
            int swap = arr[j];
            arr[j] = arr[j + 1];
            arr[j + 1] = swap;
            j--;
        }
    }
}

Now I think I've implemented it correctly (not talking about efficient or anything just the algorithm itself), but when I saw the way it was implemented in the course I'm seeing I had a bit of concerns about my implementation because they're really different and I just wanted to make sure my implementation is really an insertion sort algorithm implementation and I didn't done anything else.

Here's the implementation by the source I'm learning from:

void insertionSort(int arr[])
{
  for(int i = 1;, i < SIZE; i++)
  {
      int value = arr[i];
      int j = i - 1;
      int done = 0;
      do
      {
          if (arr[j] > value)
          {
              arr[j + 1] = arr[j];
              j--;
              if (j < 0)
                  done = 1;
          }
          else
          {
              done = 1;
          }
      } while(!done)
      arr[j + 1] = value;
  }
}

Also I would appreciate if you could compare my implementation to the one made by the source I'm learning from, any efficient differences or any other thing you think would worth mentioning.

By inspection, your implementation is incorrect: arr[0] can never be written to.  The only code that writes to arr is inside the while loop, and the body of the while loop will only execute when j > 0.

In addition, the style could be improved.  Any code of the form

int j = …;
while (some condition involving j)
{
    …
    j--;
}

would be more easily recognizable if written as a for-loop.

The body of the inner loop looks like it performs swaps with neighbouring elements.  You should be able to shift multiple elements over by one slot by doing fewer writes than that.


Counting number of messages for a mailbox over several periods The idea is to return a number of messages received today, this week, this month.

  def index
    @mailboxes = current_user.mailboxes
    @today, @month, @week, @all_time = 0, 0, 0, 0

    @mailboxes.each do |mailbox|
      @today += mailbox.messages.today.length
      @month += mailbox.messages.week.length
      @week += mailbox.messages.month.length
      @all_time += mailbox.messages.length
    end
  end

week, month scopes are similar to the following one

  scope :today, -> { where(created_at: ((Time.zone.now - 24.hours)..Time.zone.now)) }

Some suggestions ...

Use a counter cache for messages on Mailbox -- the count of all can be read from the mailbox instance.

Use #size instead of #length. This will use a counter cache for messages on Mailbox if one is present, or will use a SQL count. #length will get all of the messages back, then check the size of the array.

I would be concerned about having multiple mailboxes. If your user model has_many :messages, through: :mailbox you can run these queries more efficiently:

messages = current_user.messages

@today    = messages.today.size
@week     = messages.week.size
@month    = messages.month.size
@all_time = messages.size

With a counter cache, you're only running three queries to get the counts, and thy should be efficient enough.

Place a single index on the columns (user_id, id) on the mailboxes tables, and an index on (mailbox_id, created_at) on the messages table.

Edit: I just noticed the week and month variable assignments were round the wrong way. Also, if you wanted to be slightly more efficient you could define a scope for week_except_today and month_except_week, etc, and then:

@today    = messages.today.size
@week     = @today + messages.week_except_today.size
@month    = @week  + messages.month_except_week.size
@all_time = messages.size


Handling large amount s of ajax/POST commands I'm currently creating a utility that is heavily AJAX orientated. I have a page called ajax.php which is handling all POST requests. It switches by the provided command, checks all the arguments exist then passes it off to the relevant class e.g.

case 'addperson':
    $name = $_POST['name'] ?? die('{"result":0, "data":"name not provided"}');
    $email = $_POST['email'] ?? die('{"result":0, "data":"email not provided"}');

    try {
        (new Person(new DB()))->addPerson($name, $email);
    }
    catch (Exception $e) {
        exit("{\"result\":0, \"data\":\"{$e->getMessage()}\"}");
    }
    exit('{"result":1}');

This works fine but was wondering how people generally go about handling big lists of $_POST arguments. Would you do the above, potentially have 20 lines of $arg = $_POST['arg'] or perhaps something like,

foreach (['name', 'state'] AS $arg) {
  $$arg = $_POST[$arg] ?? die("{\"result\":0, \"data\":\"$arg not provided\"}");
}

The other option is to not check it exists at all.

list($a, $b, $c) = [$_POST['a'], $_POST['b'], $_POST['c']];

Since I'm the only one working on this project, i can ensure i always provide the necessary data but this just seems bad and may create future issues (especially if I get a few helpers!)

Finally, what do you think about having the ajax.php confirm all arguments have been provided (not validated) rather than just checking for the command and passing off the $_POST array to a class which will then confirm all arguments have been provided?

To answer the specific question: if you know the arguments in advance (as you should), you can loop over them and add them to an arguments-array instead.

Always, ALWAYS, validate input. Not just to check that it is there but also that it confirms to expectations.


Insert in circular linked list Please review my code.

public class CircularLinkedlistinsert {
    private Node first;

    private static class Node {
        int element;
        Node next;

        public Node(int element, Node next) {
            this.element = element;
            this.next = next;
        }
    }

    public void insertNodeInCircularLL(int element) {
        Node node = new Node(element, null);
        if (first == null) {
            first = node;
            first.next = first;
        } else {
            Node current = first;
            do {
                if (((current.element <= element)  && (current.next.element >= element)) || current.next == first)  {
                    node.next = current.next;
                    current.next = node;
                    if (element < first.element) {
                        first = node;
                    }
                    return;
                }
                current = current.next;
            } while (current != first);
        }
    }
}

public class CircularLinkedlistinsert {

Class names should have the first letter of each word capitalized.  I'd also change the name to remove the activity and just describe the object.  i.e., CircularLinkedList.  The method names will describe whatever action is taken.

private static class Node {
    int element;
    Node next;

While it doesn't matter too much since this is a private nested class, the element and next fields should in theory be private as well and accessed via getElement() / getNext() and setElement() / setNext() methods.  This promotes encapsulation and good object-oriented practices.  (This is a purist observation, and what you've done works fine for your model.)

public void insertNodeInCircularLL(int element)

Again, I'd rename this method for style points.  Since the entire class is a CircularLinkedList, we can just call this insertNode(), or even something as simple as add() or put().

The problem with the rest of the implementation, to me, is that you are using the element field both as the content of the Node and its position in the linked list.  That's a problem.  What if I pass in an int to the method that isn't the next logical number in the sequence?  What if I pass in something like -1?

It looks like, based on this if statement (((current.element <= element)  && (current.next.element >= element))), that you're trying to facilitate inserting a node in between two other nodes.  I think there might be a more sane way to do it, however even if you want to continue with this approach and allow the client code to pass in an index directly, you will need to do some sanity checking of the input before processing.

I think you need to have the element field be something managed internally, with only the content of the node passed in to the insert method.  This is more of a logic error in your implementation than an issue with your coding style or practices, though.

Two more advanced notes from this:

If you do end up generating the IDs yourself, you ought to look into synchronization to avoid problems with multithreading.
This CircularLinkedList can only accept nodes which hold int information.  You could change the element field to be a true id, and then use Generics to make your implementation truly powerful.


Recursive permutation of an array Basically, this is a recursive function to generate all of the permutations of an array.

The idea is this:

recursive case: start at the specified array index, and make a case for starting the next index (incremented one) for each of the indexes that come after the specified index by swapping the index with the next if not the same.

base case: the array has has no indexes greater than the current index. store the array, and increment the number of permutations found.

Here's what I thought would work:

public static void <T> permute (T[][] permutations, T[] array)
{
    if (permutations.length != factorial(array.length))
        throw new IllegalArgumentException();
    permute(permutations, array, 0, 0);
}
private static int <T> permute(
        T[][] permutations,
        T[] array,
        int midStart,
        int count)
{
    if (midStart == array.length)
    {
        permutations[count] = Arrays.copyOf(array, array.length);
        return count + 1;
    }
    else
    {
        for (int i = midStart; i < array.length; i++)
        {
            T temp = array[i];
            array[i] = array[midStart];
            array[midStart] = temp;
            count = permute(permutations, array, midStart + 1, count);
        }
        return count;
    }
}

But this doesn't work because array being passed on the recursive call is being altered before the caller swaps the values again.

What's an efficient way to do this without needing to change it to something like so (without the extra space to store another array at each function call)?

private static <T> int permute(
        T[][] permutations,
        T[] array,
        int midStart,
        int count)
{
    if (midStart == array.length)
    {
        permutations[count] = Arrays.copyOf(array, array.length);
        return count + 1;
    }
    else
    {
        for (int i = midStart; i < array.length; i++)
        {
            T temp = array[i];
            array[i] = array[midStart];
            array[midStart] = temp;
            count = permute(
                        permutations,
                        Arrays.copyOf(array, array.length),
                        midStart + 1,
                        count);
        }
        return count;
    }
}

permutations[count] = Arrays.copyOf(array, array.length);
        return count + 1;

Here you are copying the array into the permutations array after permuting the array itself.  This is why you have to keep making copies of the array.

        return count;

If you instead do nothing in this case, you can change

    else
    {
        for (int i = midStart; i < array.length; i++)
        {
            T temp = array[i];
            array[i] = array[midStart];
            array[midStart] = temp;
            count = permute(permutations, array, midStart + 1, count);
        }
        return count;
    }

to copy the array before recursing instead.

        count = permute(permutations, array, midStart + 1, count);
        for (int i = midStart+1; i < array.length; i++)
        {
            permutations[count] = Arrays.copyOf(array, array.length);
            permutations[count][i] = array[midStart];
            permutations[count][midStart] = array[i];
            count = permute(permutations, permutations[count], midStart + 1, count + 1);
        }

        return count;

Now you never change the original array and you don't have to do any extraneous copies.  A side effect of this is that you don't need a temporary variable to do the swap either.

Note that I also changed the for loop not to do the no-op swap at the beginning.  This was necessary since we don't want to copy in that case.

We're now one copy short, so change

public static void <T> permute (T[][] permutations, T[] array)
{
    if (permutations.length != factorial(array.length))
        throw new IllegalArgumentException();
    permute(permutations, array, 0, 0);
}

to

public static <T> void permute (T[][] permutations, T[] array)
{
    if (permutations.length != factorial(array.length)) {
        throw new IllegalArgumentException();
    }

    permutations[0] = Arrays.copyOf(array, array.length);
    permute(permutations, array, 0, 1);
}


Python calculator taking string input It accepts string input like '1+1' instead of num1 = 1, num2 = 2, operation = '+' or something like that.

def Numbers(var):
    return var == '0' or var == '1' or var == '2' or var == '3' or var == '4' or var == '5' or var == '6' or var == '7' or var == '8' or var == '9'

def Test4Num(varstr):
    n = 0
    var = ''
    try:
        while Numbers(varstr[n]):
            var += varstr[n]
            n += 1
    except: pass
    return (int(var), n)

def operation(string, num1, num2):
    if string == '+':
        return num1 + num2
    if string == '-':
        return num1-num2
    if string == '*':
        return num1*num2
    if string == '/':
        return num1/num2
    if string == '^':
        return num1 ** num2

def operator(operato):
    return operato == '+' or operato == '-' or operato == '*' or operato == '/' or operato == '^'

negate = False
char = input('')

print(char + '=')
while True:
    try:
        if char[0] == '-': #for negative numbers
            negate = True #because here the numbers are string format
            char = char[1:]
        number1 = Test4Num(char)[0]
        if negate == True:
            number1 = -number1
            negate = False
        end_number1 = Test4Num(char)[1]
        char = char[end_number1:]
        if char == '':
            print(number1)
            break
        op = char[0]
        char = char[1:]
        number2 = Test4Num(char)[0]
        end_number2 = Test4Num(char)[1]
        result = operation(op, number1, number2)
        number1 = result
        char = str(number1) + char[end_number2:]
        print(char)
    except: break
#created by Estoria Wandertag, while he was sitting on the toilet and thought about life and programming

This code is pretty straightforward, I mean it doesn't use class, any imports, etc. (ok, it can just use the 5 main operations in math, but that's extendable) and even though it's easy, I didn't find it on the internet...
So that's the reason I wanted to share this. If you have ideas, please share them. I'm pretty new here and also I haven't been using Python for very long, so there are plenty of things to add, I guess. (Sorry for my awful English, I'm German)

On top of the other very good answers:

Code organisation and tests

It is a good idea to separate the logic about input/output from the logic computing results. In your case, you could have a function taking an expression as a string and returning a number.

This makes your code easier to understand, easier to reuse and more testable.

Also, it is a good habit to put your code actually calling the logic behind an if __name__ == "__main__": guard.

All of this taken into account, you have:

def eval_math_expr(expr):
    negate = False
    while True:
        try:
            if expr[0] == '-': #for negative numbers
                negate = True #because here the numbers are string format
                expr = expr[1:]
            number1 = Test4Num(expr)[0]
            if negate == True:
                number1 = -number1
                negate = False
            end_number1 = Test4Num(expr)[1]
            expr = expr[end_number1:]
            if expr == '':
                return number1
            op = expr[0]
            expr = expr[1:]
            number2 = Test4Num(expr)[0]
            end_number2 = Test4Num(expr)[1]
            result = operation(op, number1, number2)
            number1 = result
            expr = str(number1) + expr[end_number2:]
        except Exception as e:
            print(e)
            break
    return number1

if __name__ == '__main__':
    interactive = False
    if interactive:
        expr = input('Enter your expression:')
        print(expr + '=')
        print(eval_math_expr(expr))
    else:
        for expr, res in {"2": 2, "2*4": 8, "4+8": 12, "100/3": 33, "2^3": 8}.items():
            result = eval_math_expr(expr)
            if res != result:
                print("Computing", expr, "got", result, "instead of", res)

I also took this chance to remove the silent catch of exceptions which makes things hard to understand when an error occurs.

By the way, bare except (with no explicit exception type caught) and ignored exceptions are usually frowned upon as they make debugging very painful.

Naming

Your function names are not very good. Your could define is_digit, get_number, perform_operation.

Simplify is_digit function

The best data structure for the check you want to perform is a set.
You could write something like:

DIGITS = set('0123456789')

def is_digit(var):
    return var in DIGITS

Simplify perform_operation

The best data structure for the logic you want to perform is a dictionnary mapping operators to functions.

import operator

OPERATIONS = {
    '+' : operator.add,
    '-' : operator.sub,
    '*' : operator.mul,
    '/' : operator.floordiv,
    '^' : operator.pow,
}

def perform_operation(string, num1, num2):
    op = OPERATIONS.get(string, None)
    if op is not None:
        return op(num1, num2)
    else:
        return None  # How to handle this?

Note: using floordiv instead of truediv fixes an issue found with tests mentionned above.

Simplify get_number

In Python, you usually don't need to get elements from an object by index. What you want is usually to iterate over the different elements. I highly recommand reading/watching Ned Batchelder talk called "Loop Like A Native". In your case, you could write something like:

def get_number(varstr):
    s = ""
    for c in varstr:
        if not is_digit(c):
            break
        s += c
    return (int(s), len(s))

Using tuple unpacking

As already said, you could use number1, end_number1 = get_number(expr) to call get_number only once.

At this stage, the code looks like:

import operator

DIGITS = set('0123456789')
OPERATIONS = {
    '+' : operator.add,
    '-' : operator.sub,
    '*' : operator.mul,
    '/' : operator.floordiv,
    '^' : operator.pow,
}

def is_digit(var):
    return var in DIGITS

def get_number(varstr):
    s = ""
    for c in varstr:
        if not is_digit(c):
            break
        s += c
    return (int(s), len(s))

def perform_operation(string, num1, num2):
    op = OPERATIONS.get(string, None)
    if op is not None:
        return op(num1, num2)
    else:
        return None  # How to handle this?

def eval_math_expr(expr):
    negate = False
    while True:
        try:
            if expr[0] == '-': #for negative numbers
                negate = True #because here the numbers are string format
                expr = expr[1:]
            number1, end_number1 = get_number(expr)
            expr = expr[end_number1:]
            if negate == True:
                number1 = -number1
                negate = False
            if expr == '':
                return number1
            op = expr[0]
            expr = expr[1:]
            number2, end_number2 = get_number(expr)
            result = perform_operation(op, number1, number2)
            number1 = result
            expr = str(number1) + expr[end_number2:]
        except Exception as e:
            print(e)
            break
    return number1

if __name__ == '__main__':
    interactive = False
    if interactive:
        expr = input('Enter your expression:')
        print(expr + '=')
        print(eval_math_expr(expr))
    else:
        for expr, res in {"2": 2, "2*4": 8, "4+8": 12, "100/3": 33, "2^3": 8, "-2": -2, "-2-3": -5}.items():
            result = eval_math_expr(expr)
            if res != result:
                print("Computing", expr, "got", result, "instead of", res)

About negate

You set negate to False at the very beginning and make sure you reset it back to False at each iteration so that the next iteration works fine. It would be clearer to initialise it to False at the beginning of the iteration. You could also set it to negate = expr[0] == '-'.

        negate = expr[0] == '-' #for negative numbers
        if negate:
            expr = expr[1:]
        number1, end_number1 = get_number(expr)
        expr = expr[end_number1:]
        if negate:
            number1 *= -1

An alterative could be to handle this as part of the get_number function.

def get_number(varstr):
    s = ""
    if varstr[0] == '-':
        s += "-"
        varstr = varstr[1:]
    for c in varstr:
        if not is_digit(c):
            break
        s += c
    return (int(s), len(s))

def eval_math_expr(expr):
    while True:
        try:
            number1, end_number1 = get_number(expr)
            expr = expr[end_number1:]
            if expr == '':
                return number1
            op = expr[0]
            expr = expr[1:]
            number2, end_number2 = get_number(expr)
            number1 = perform_operation(op, number1, number2)
            expr = str(number1) + expr[end_number2:]
        except Exception as e:
            print(e)
            break
    return number1

The whole code is:

import operator

DIGITS = set('0123456789')
OPERATIONS = {
    '+' : operator.add,
    '-' : operator.sub,
    '*' : operator.mul,
    '/' : operator.floordiv,
    '^' : operator.pow,
}

def is_digit(var):
    return var in DIGITS

def get_number(varstr):
    s = ""
    if varstr[0] == '-':
        s += "-"
        varstr = varstr[1:]
    for c in varstr:
        if not is_digit(c):
            break
        s += c
    return (int(s), len(s))

def perform_operation(string, num1, num2):
    op = OPERATIONS.get(string, None)
    if op is not None:
        return op(num1, num2)
    else:
        return None  # How to handle this?

def eval_math_expr(expr):
    while True:
        try:
            number1, end_number1 = get_number(expr)
            expr = expr[end_number1:]
            if expr == '':
                return number1
            op = expr[0]
            expr = expr[1:]
            number2, end_number2 = get_number(expr)
            number1 = perform_operation(op, number1, number2)
            expr = str(number1) + expr[end_number2:]
        except Exception as e:
            print(e)
            break
    return number1

if __name__ == '__main__':
    interactive = False
    if interactive:
        expr = input('Enter your expression:')
        print(expr + '=')
        print(eval_math_expr(expr))
    else:
        for expr, res in {"2": 2, "2*4": 8, "4+8": 12, "100/3": 33, "2^3": 8, "-2": -2, "-2-3": -5}.items():
            result = eval_math_expr(expr)
            if res != result:
                print("Computing", expr, "got", result, "instead of", res)

Reorganisation in the main function

At the moment, the code parses two number and an operation, computes the operation, convert the result into a string to be parsed at next iteration. Thus, each temporary result gets converted from integer to string then integer. This leads to issues when the result cannot be converted back and forth - for instance when you perform divisions with non-integer result. Also this could be optimised a lot with the following logic:

get the first number and store it as the current number
in each iteration: get the operation and the next number. Compute the operation and store the result as the current number, to be reused at next iteration.
at the end, return the current number.

You could write this:

def eval_math_expr(expr):
    n, end_n = get_number(expr)
    expr = expr[end_n:]
    while expr:
        op, expr = expr[0], expr[1:]
        n2, end_n = get_number(expr)
        n = perform_operation(op, n, n2)
        expr = expr[end_n:]
    return n


Mergesort with std::vector I made a mergesort implementation based on this code. Please review my C++ version.

using size = std::size_t;

template<typename T>
void merge(std::vector<T>& v, size low, size mid, size high) {
    size i = low, j = mid+1;
    std::vector<T> old{v};
    for (size x = low; x <= high; ++x) {
        if (i > mid) {
            v[x] = old[j++];
        } else if (j > high){
            v[x] = old[i++];
        } else if (old[j] < old[i]) {
            v[x] = old[j++];
        } else {
            v[x] = old[i++];
        }
    }
}

template<typename T>
void sort(std::vector<T>& v, size low, size high) {
    if (high <= low) {
        return;
    }
    size mid = (high+low)/2;
    sort(v,0,mid);
    sort(v,mid+1,high);
    merge(v,low,mid,high);
}

template<typename T>
void sort(std::vector<T>& v) {
    if (v.size() < 2) {
        throw std::out_of_range("out of range");
    }
    sort(v,0,v.size()-1);
}

You have decided your ranges are inclusive:

sort(v,0,v.size()-1);

So low=>lowest member, high=>highest member.
Most C++ code uses the one past the end system (see iterators). Not a huge deal. It just means you need to use operator<= rather than operator< and you have to sprinkle a couple of +1 around.

Your main merge loop is easier to write if you concentrate on what it is supposed to do (Compare items from the two ranges).

while(i <= mid && j <= high) {
    if (old[j] < old[i]) {
        v[x++] = old[j++];
    } else {
        v[x++] = old[i++];
    }
}

At this point one of the ranges is exhausted, so all the elements must come from the other range. So just copy them.

while(i <= mid) {
   v[x++] = old[i++];
}
while(j <= high) {
   v[x++] = old[j++];
}

But we can also replace manual loops using algorithms:

std::copy(&old[i], &old[mid+1],  &v[x]); x+= (mid-i)+1;
std::copy(&old[j], &old[high+1], &v[x]);

Also, you are copying the whole array into old when you are only going to alter a subset of it. Say the vector contains 10,000 elements. But this merge you are merging low=>5, mid=>6, high=>7 that seems like a lot of extra copying.

I would simplify this and sort into an external array, then copy only the sorted elements back.

std::vector<T>  sorted(high-low+1);
while(i <= mid && j <= high) {
    if (old[j] < old[i]) {
        sorted[x++] = v[j++];
    } else {
        sorted[x++] = v[i++];
    }
}
std::copy(&v[i], &v[mid+1],  &sorted[x]); x+= (mid-i)+1;
std::copy(&v[j], &v[high+1], &sorted[x]);

std::copy(sorted.begin(), sorted.end(), &v[low]);

Since we are in the era of C++14, we can do a lot better than copying the array. We can introduce the concept of a move and move the elements around. For integers there is no difference, but if T is a big object, we would rather move it if we can.

        sorted[x++] = v[j++];

        // becomes.

        sorted[x++] = std::move(v[j++]);

Rather than passing the index and the container, just pass iterators (now it becomes easier to use the one past the end metaphor). Also, we can generalize the container to a C so that you can pass any container.

The result is:

template<typename I>
void merge(I low, I mid, I high) {
    I i   = low;
    I j   = mid;

    typedef typename I::value_type  value_type;
    std::vector<value_type> sorted(std::distance(low, high));
    auto x = sorted.begin();

    while(i < mid && j < high) {
        if (*j < *i) {
            // Use move semantics. So we don't copy expensive T object around.
            (*x) = std::move(*j); ++j;++x;
        }
        else {
            // Put the ++ inline to show difference in readability.
            // Prefer the above to this.
            (*x++) = std::move(*i++);
        }
    }
    // Don't use a manual loop when you can use an already optimized algorithm
    x = std::move(i, mid,  x);
    x = std::move(j, high, x);

    // Now move the sorted values back.
    std::move(std::begin(sorted), std::end(sorted), low);
}

template<typename I>
// requires std::RandomAccessIterator<I>           // Proposed C++17 syntax for concepts.
//       && std::LessThanComparable<typename I::value_type>>
void sort(I low, I high) {
    size size = std::distance(low, high);
    if(size <= 1) {
        return;
    }
    I    mid(low)
    std::advance(mid, size/2);
    sort(low, mid);
    sort(mid, high);
    merge(low, mid, high);
}

template<typename C>
void sort(C& v) {
    sort(std::begin(v), std::end(v));
}


Convert a double list to a grouped string The program has an input a list of doubles and the output needs to be a string containing the list values grouped by their value. The list values will be grouped if they are equal. Something like: input 9,77,5,5,31 => output 9 77 2*5 31

I created an algorithm in C# (in Java I think that is almost the same) for this but I am not sure if it can be improved regarding its speed or code quaility, or if it has some bugs that I could not see. The algorithm having also some more input, output examples is below.

    List<double> input = new List<double> { 11, 32, 32, 43}; // output 11 2*32 43
    //List<double> input = new List<double> { 11, 11, 43, 43 }; // output 2*11 2*43
    //List<double> input = new List<double> { 10, 11, 12, 13, 14, 15, 16 }; // output 10 11 12 13 14 15 16
    //List<double> input = new List<double> { 11, 11, 11, 11, 11 }; // output 5 * 11
    //List<double> input = new List<double> { 11, 11, 32, 22, 22, 22, 4, 10, 10 }; // output 2*11 32 3*22 4 2*10

    string listAsString = string.Empty;
    double nextElem = double.MinValue;
    for (int i = 0; i < input.Count; i++)
    {
        double currentElem = input[i];

        if (i + 1 < input.Count)
        {
            nextElem = input[i + 1];
        }

        int equalCount = 0;
        while (currentElem.Equals(nextElem) && i < input.Count)
        {
            equalCount++;
            i++;
            currentElem = nextElem;

            if (i < input.Count)
            {
                nextElem = input[i];
            }
        }

        if (equalCount < 2)
        {
            listAsString += currentElem + " ";
        }
        else
        {
            listAsString += equalCount + "*" + currentElem + " ";
            i--;
        }
    }

    Console.WriteLine(listAsString);

Please let me know if you noticed some bugs or see some improvements that can be done.

Also if you know another implementation of this requirement please add it so that a comparation regarding results, speed, code quality between the algorithms can be done... and find the best way to handle this.

Disclaimer: Edit
OP later specified that the numbers must be grouped only if they are adjacent. My code does not do that. It groups all equal elements. Please refer to @Xiaoy312's answer for that. I have my own solution, but as that is not a significant improvement over @Xiaoy312's I'm not posting it. To me, @Xiaoy312's answer is overkill for the specific use case the OP mentions, it promotes good programming practices and code reusability.
Old answer
What you're trying to achieve here is similar to Run-Length Encoding, a basic data compression technique.
Off the bat, I'll make a few observations:

You are repeatedly concatenating immutable strings. Usually not a great idea, as it involves large overheads in creating and discarding a whole lot of string objects. Use StringBuilder in the System.Text namespace instead, and append to it instead of doing repeated string concatenation.

Too much code in one function. Split it up! An obvious candidate is the part which counts the number of occurrences of a value in the list. Try to follow separation of concerns and the Single Responsibility Principle.

You do redundant counting for values. For a particular value, you should cache its count and not repeat the count for the same value again (memoization).

Try LINQ. It might even make this code into a one-liner. And be more idiomatic at the same time, if a bit slower.

You could do this better by doing the logic in 2 passes - in the first, you construct a map between the unique values in the list and their count, and in the second, you use this map to generate the string representation.

Ideally for comparing 2 doubles for equality, keeping in mind their error margins and inexact representation of certain values, you would check that the modulus of the difference of the 2 values was less than some small epsilon. However, for code like this, that's probably overkill.

Try to program as generic as possible. Use an IEnumerable<T> instead of a very specific List<double>, you will be able to use the resulting code for many different types of collections. This also involves typing to interfaces instead of concrete classes.

Using most of the above suggestions, herein follows the code (test fiddle here):
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

public class RLEofList
{

    private static Dictionary<double, int> CountMap(List<double> input)
    {
        var countMap = new Dictionary<double, int>(input.Count);
        foreach(double item in input)
        {
            if(!countMap.ContainsKey(item))
            {
                countMap.Add(item, input.Count(x => x == item));
            }
        }
        return countMap;
    }

    public static string ListToRLEString(List<double> input)
    {
        var countMap = CountMap(input);
        var accumulator = new StringBuilder(input.Count);//rough guess for capacity of StringBuilder
        foreach(KeyValuePair<double, int> itemWithCount in countMap)
        {
            if(itemWithCount.Value > 1)
            {
                accumulator
                .Append(itemWithCount.Value)
                .Append("*");
            }
            accumulator
            .Append(itemWithCount.Key)
            .Append(" ");
        }
        return accumulator.ToString();
    }

    public static void Main()
    {
        var list = Console
            .ReadLine()
            .Split(',')
            .ToList()
            .ConvertAll<double>(x => Double.Parse(x));
        Console.WriteLine(ListToRLEString(list));
    }
}

The piece of code above will accept input as a single line of comma-separated numbers as per the example in the question. You might need to use Regex.Split for more complicated input patterns.
I'm not collapsing this code into a LINQ one-liner as I feel this is more educational.


Thin proxy class for hashables I need some kind of thin wrapper object to mark dictionary keys, like:

d = {
  Required('name'): str,
  Optional('age'): int,
}

And these wrappers should behave like the wrapped object (comparison, hashing, etc):

marked = Required('name')
marked == 'name'  #-> True
d[marked] = 'hello'
d['name']  #-> 'hello'

With a single additional property: it should remember the added class:

isinstance(marked, Required)  #-> True

I dislike those heavyweight Proxy Patterns which mimic all special properties and thinking of the following idea instead:

class Wrapper(object):
    def __new__(cls, value):
        value_type = type(value)
        Type = type(
            value_type.__name__,  # same name
            (cls, value_type),  # Wrapper + bases
            {})
        Type.__new__ = value_type.__new__  # prevent recursion
        return Type(value)

    # Override method
    def __repr__(self):
        return 'Wrapper({})'.format(self)

This basically instantiates the type again, but with an additional base, and seems to work:

v = 'abc'  # Wrapped value
vt = type(v)  # Wrapped value type
w = Wrapper(v)

assert w == v  # equality
assert isinstance(w, vt)  # type matches
assert isinstance(w, Wrapper)  # and is Wrapper
assert hash(w) == hash(v)  # hash matches (required for dict keys)

Can you think of any disadvantages of this approach?

You asked, "Can you think of any disadvantages of this approach?" One obvious problem is that this requires you to construct a new class each time the wrapper is called, which potentially ends up wasting a lot of memory. So you would want to cache and reuse these classes.

class Wrapper(object):
    # A map from (value type, wrapper class) to wrapped class
    __cache = {}

    def __new__(cls, value):
        value_type = type(value)
        bases = value_type, cls
        try:
            wrapper_class = cls.__cache[bases]
        except KeyError:
            wrapper_class = type(value_type.__name__, bases, {})
            cls.__cache[bases] = wrapper_class
        return wrapper_class(value)

Note that this is a rare case where we actually want to use a private name — the idea is to avoid a name clash when wrapping a class that already has a cache (or _cache, or whatever name we pick) attribute.

I can't help feeling, though, that Martijn Pieters is right in the Stack Overflow version of this question: if you just want to proxy a dictionary key, then you just need to proxy the __eq__ and __hash__ special methods. Keep it simple!


Version comparison function I have this version comparison function in java.

Any cleaner way to write this without all the nested 'if' statements?

Note that the suggestions must be in Java code only.

@Override
public int compareTo(Version o)
{
    int result = this.major.compareTo(o.major);
    if (result == 0)
    {
        result = this.minor.compareTo(o.minor);
        if (result == 0)
        {
            result = this.minor2.compareTo(o.minor2);
            if (result == 0)
            {
                result = this.suffix.compareTo(o.suffix);
            }
        }
    }

    return result;
}

Edit:

Just to be clear on the 'type' of the fields of the Version class:

Integer major, minor, minor2;
String suffix;

One point which I may suggest is reorder if statements to reduce indentation:

@Override
public int compareTo(Version o)
{
    int result = this.major.compareTo(o.major);
    if (result != 0)
    {
        return result;
    }

    result = this.minor.compareTo(o.minor);
    if (result != 0)
    {
        return result;
    }

    result = this.minor2.compareTo(o.minor2);
    if (result != 0)
    {
         return result;
    }

    return this.suffix.compareTo(o.suffix);
}

Also, I think you can omit this keyword.


Stack implementation using an array I am trying a stack implementation using an array. I want to know if this approach is OK or if there is a logical problem. This program is working fine.

#include <stdio.h>
#define MAXSIZE 5

struct stack            /* Structure definition for stack */
{
    int stk[MAXSIZE];
    int top;
} s;

void push ();
void pop();
void display ();
void search();

int main(){

int element, choice;

s.top = -1;

clrscr();

while (1)
    {
        printf ("------------------------------------------\n");
        printf ("1. PUSH\n");
        printf ("2. POP\n");
        printf ("3. DISPLAY\n");
        printf ("4. SEARCH\n");
        printf ("5. EXIT\n");
        printf ("------------------------------------------\n");

        printf ("Enter your choice\n");
        scanf   ("%d", &choice);

        switch (choice)
        {
            case 1: push();
                break;
            case 2: pop();
                break;
            case 3: display();
                break;
            case 4: search();
                break;
            case 5: return;
        }

}

}

/*Function to add an element to the stack*/
void push() {
    int num;

    if (s.top == (MAXSIZE - 1))
    {
        printf ("Error: Overflow\n");
    }
    else
    {
        printf ("Enter the element to be pushed\n");
        scanf ("%d", &num);
        s.top = s.top + 1;
        s.stk[s.top] = num;
    }
}

/*Function to delete an element from the stack*/
void pop ()
{
    int num;
    if (s.top == - 1)
    {
        printf ("Error: Stack Empty\n");
    }
    else
    {
        num = s.stk[s.top];
        printf ("poped element is = %d\n", num);
        s.top = s.top - 1;
    }
}

/*Function to display the status of the stack*/
void display()
{
    int i;
    if (s.top == -1)
    {
        printf ("Error: Stack Empty\n");
    }
    else
    {
        printf ("\nItems in Stack\n");
        for (i = s.top; i >= 0; i--)
        {
            printf ("%d\n", s.stk[i]);
        }
    }
    printf ("\n");
}

void search(){

    int i;
    int num;
    int counter=0;
    if (s.top == -1)
    {
        printf ("Error: Stack Empty\n");
    }
    else
    {
        printf ("Enter the element to be searched\n");
        scanf ("%d", &num);

        for (i = s.top; i >= 0; i--)
        {
            if(num==s.stk[i]) {
            counter++;
            break;
            }
        }

        if(counter>0){
            printf ("Element %d found in stack\n", num);
        }
        else {
            printf ("Element %d not found in stack\n", num);
        }
    }
    printf ("\n");

}

sscanf is not safe

It's not part of your stack but part of your driver code, but scanf() is not a good function to use.  There are many reasons for this, but the basic problem in this case is that if the user enters a letter rather than a digit for input in push() for example, the program will loop forever.

Better, in this case, would be to use something like getchar().

separate stack operations from input/output

Having a function like push() only do one thing (pushing an item to the stack) is generally better design because it makes it easier to re-use your code.  In this code, push() cannot be easily re-used because it is tied directly to user I/O.  Better would be to cleanly separate all stack operations from the I/O to better foster re-use.

operations should operate on passed parameters

In this code, there can only be one stack because it's globally defined and used by all stack operation functions.  A better scheme would be to pass the address of the stack structure to each function to make it more flexible and more general.

Additionally, a function like push() should also require an argument that speccifies what is to be pushed onto the stack, and a function like pop() should return either a copy of or a pointer to the data item that was popped from the stack.

use C idioms to make the code shorter and easier to read

Writing code such as

s.top = s.top + 1;
s.stk[s.top] = num;

is not incorrect, but is not idiomatic C.  The C way of writing that would be

s.stk[++s.top] = num;

Writing code in this way will make it easier for others to understand you, in the same way that speaking, say, idiomatic Italian will make it easier for native Italian speakers to understand you.

avoid non-portable code

The clrscr() call is not in <stdio.h> and is not portable.  I had to comment out that line to get the code to compile and run.  Ideally, you'd avoid all non-portable code.  If you can't avoid that, however, you should state what dependencies the code has and also include the header files that clearly indicate those dependencies.

think about the user of your code

It's good that you avoided using "magic numbers" and instead you wisely chose to define MAXSIZE to indicate the size of your stack.  However, it would be useful to have a comment indicating what it's for, and also to maybe rename it to something a little more descriptive, such as MAXSTACKSIZE.

avoid single-letter variable names

While i or j might be perfectly fine variable names for looping indexes, naming your stack s is not very user-friendly.  We are long beyond the days that each line of code had to fit on an 80-character punch card!

I hope these items inspire you to write more code, and to strive to continuously improve the quality of the code that you write.


Optimize YUV channel splitting function The input pointer *data contains the data that needs to be split into different arrays and put in yuvInput.  Each pixel is 32 bytes 4 * 8.  Note the (j*4) to break it up per pixel segment.  The 4th byte is the alpha channel which gets skipped (the reason there is no idx+3).

This method needs optimization.  If anyone is willing to help, please do.

void SplitYUVPlanes(int width, int height, unsigned char *data, int size, unsigned char *yuvInput[3])
{
    // live input *data is YUV444 Packed
    // Conversion from 444 Packed -> 444 Planar
    int index = 0;
    int srcStride = size;

    // need to flip image from bottom-up to top-down
    int revheight = height - 1;

    unsigned char* pLuma = yuvInput[0];
    unsigned char* pChromaU = yuvInput[1];
    unsigned char* pChromaV = yuvInput[2];

    for (int i = 0; i < height; ++i)
    {
        // read bottom line first
        int line = (revheight - i) * srcStride;

        for (int j = 0; j < width; ++j)
        {
            int idx = line + (j * 4);
            pLuma[index] = data[idx + 2]; //Y
            pChromaV[index] = data[idx + 1]; //V
            pChromaU[index] = data[idx + 0]; //U
            index++;
        }
    }
}

Your multiplication expressions aren't ideal; instead (which might be faster) you could add 4 and subtract srcStride from the previous value at the top of each loop.

Apart from that (and this is just a guess) I guess some other things might in theory make this faster.

Have three loops: write to Luma in the 1st loop, then write to ChromaV in the 2nd loop, and write to ChromaU in the 3rd loop.
Calibrate the length of (number of bytes moved in) each loop, so that the chunk of input data stays in cache for each of the three loops
Use some kind of read-ahead, to ensure that the byte is read before it needs to be written, for example:

Read 1st byte
Read 2nd byte
Read 3rd byte
Read 4th byte
Write 1st byte
Read 5th byte
Write 2nd byte
etc.

Read 4 or 8 bytes at a time, for example into a struct which contains 4 or 8 byte-fields and is a union with an int32 or int64 field.


Two AngularJS controllers for camera actions I have the same functionality in two controllers and I want to refactor to place it in a shared method that both controllers can access. I was thinking on creating a separate service called "Camera" and inject that service as a dependecy into both controllers as I'm currently doing with "Login". Is this a good approach?

  .controller('pictureOfTheDamageCtrl', function ($scope, $cordovaCamera, $ionicPopup, $rootScope, $timeout, Login, $ionicLoading) {
    $rootScope.width = document.getElementsByTagName('ion-content')[0].clientWidth;

    $scope.takePicture = function () {
      var options = {
        quality: 75,
        destinationType: Camera.DestinationType.DATA_URL,
        sourceType: Camera.PictureSourceType.CAMERA,
        allowEdit: false,
        targetWidth: 1024,
        targetHeight: 567,
        encodingType: Camera.EncodingType.JPEG,
        popoverOptions: CameraPopoverOptions,
        saveToPhotoAlbum: false
      };

      $cordovaCamera.getPicture(options).then(function (imageURI) {
        $rootScope.damage = "data:image/jpeg;base64," + imageURI;
      }, function (err) {
        $ionicPopup.alert({
          title: 'Camera',
          template: 'Something went wrong: ' + err
        });
      });
    }

  })
  .controller('pictureOfTheVehicleCtrl', function ($scope, $cordovaCamera, $ionicPopup, $rootScope, $timeout, Login, $ionicLoading) {
    $rootScope.width = document.getElementsByTagName('ion-content')[0].clientWidth;

    $scope.takePicture = function () {
      var options = {
        quality: 75,
        destinationType: Camera.DestinationType.DATA_URL,
        sourceType: Camera.PictureSourceType.CAMERA,
        allowEdit: false,
        targetWidth: 1024,
        targetHeight: 567,
        encodingType: Camera.EncodingType.JPEG,
        popoverOptions: CameraPopoverOptions,
        saveToPhotoAlbum: false
      };

      $cordovaCamera.getPicture(options).then(function (imageURI) {
        $rootScope.vehicle = "data:image/jpeg;base64," + imageURI;
      }, function (err) {
        $ionicPopup.alert({
          title: 'Camera',
          template: 'Something went wrong: ' + err
        });
      });
    }

  })

Looks ok, but here are some suggestions.

Your picture configuration looks the same for both controllers. You could move that out to an angular constant/value. If you anticipate the API mutating that config object, you can always shallow-copy them with Object.assign.

Next, proceed with your service. I'd move everything into it. All your controller has to do is call the API and receive the data URL. Move the ionic popup warning into the service as well for a centralized handling of the popup.

Another thing is the use of $rootScope. It's a quick way to share values, but like globals, $rootScope is modifiable everywhere. Your app is depending on data that may or may not be there and depends on changes whose origins you may not know. For small apps, that's fine as two-way binding is really handy but just in case your app becomes big, you might consider moving off data into a service/factory to draw a line between data and presentation.

.value('PICTURE_SETTINGS', {
  quality: 75,
  destinationType: Camera.DestinationType.DATA_URL,
  sourceType: Camera.PictureSourceType.CAMERA,
  allowEdit: false,
  targetWidth: 1024,
  targetHeight: 567,
  encodingType: Camera.EncodingType.JPEG,
  popoverOptions: CameraPopoverOptions,
  saveToPhotoAlbum: false
})
.factory('cameraFactory', function($cordovaCamera, $ionicPopup, PICTURE_SETTINGS){

  return {
    takePicture: function(){
      $cordovaCamera.getPicture(Object.assign({}, PICTURE_SETTINGS)).then(function(imageURI) {
        return "data:image/jpeg;base64," + imageURI;
      }, function(err) {
        $ionicPopup.alert({
          title: 'Camera',
          template: 'Something went wrong: ' + err
        });
      });
    }
  }
})
.controller('pictureOfTheDamageCtrl', function($scope, $rootScope, cameraFactory) {
  $rootScope.width = document.getElementsByTagName('ion-content')[0].clientWidth;

  scope.takePicture = function() {
    cameraFactory.takePicture().then(function(imageURI) {
      $rootScope.damage = imageURI;
    });
  }

})
.controller('pictureOfTheVehicleCtrl', function($scope, $rootScope, cameraFactory) {
  $rootScope.width = document.getElementsByTagName('ion-content')[0].clientWidth;

  $scope.takePicture = function() {
    cameraFactory.takePicture().then(function(imageURI) {
      $rootScope.vehicle = imageURI;
    });
  }

})


RSpec tests for a cash flow model This is my first model RSpec test:

require 'spec_helper'

describe CashFlow do
  context 'DB Fields' do
    it { should have_db_column :amount_cents }
    it { should have_db_column :amount_currency }
    it { should have_db_column :user_id }

    it { should have_db_column :target_date }
    it { should have_db_column :created_at }
    it { should have_db_column :updated_at }
  end

  context 'Associations' do
    it { should belong_to :user }
    it { should have_and_belong_to_many :tags }
  end

  context 'Validation' do
    it { should validate_presence_of :amount_cents }
    it { should validate_numericality_of :amount_cents }
    it { should ensure_exclusion_of(:amount_cents).in_array([0,]) }

    it { should validate_presence_of :amount_currency }
    it { should validate_presence_of :target_date }

    it { should validate_presence_of :user }
  end

  context 'Scopes' do
    subject { CashFlow }

    context 'Incoming/Outcoming flows' do
      let(:incoming_cash_flows) { 4.times.map { create :cash_flow } }
      let(:outcoming_cash_flows) { 4.times.map { create :cash_flow, negative: true } }

      it 'should be able to return all and only incomings cash_flows' do
        expect(subject.incoming).to include *incoming_cash_flows
        expect(subject.incoming).not_to include *outcoming_cash_flows
      end

      it 'should be able to return all and only outcomings cash_flows' do
        expect(subject.outcoming).to include *outcoming_cash_flows
        expect(subject.outcoming).not_to include *incoming_cash_flows
      end
    end

    context 'Past/Future flows' do
      [-1, nil, 1].each do |num|
        _threshold = num ? Date.today + num.year : nil

        let(:threshold) { _threshold }
        let(:future_cash_flows) { 4.times.map { create :cash_flow, threshold: threshold } }
        let(:past_cash_flows) { 4.times.map { create :cash_flow, past: true, threshold: threshold } }

        it "should be able to return all and only past cash_flows with #{_threshold || 'current date (default)'} threshold" do
          expect(subject.past(threshold)).to include *past_cash_flows
          expect(subject.past(threshold)).not_to include *future_cash_flows
        end

        it "should be able to return all and only future cash_flows with #{_threshold || 'current date (default)'} threshold" do
          expect(subject.future(threshold)).to include *future_cash_flows
          expect(subject.future(threshold)).not_to include *past_cash_flows
        end
      end
    end
  end

  it 'should save specified currency' do
    %w(GEL RUB USD EUR).each do |currency|
      cash_flow_id = create(:cash_flow, currency: currency).id

      cash_flow = CashFlow.find(cash_flow_id)
      expect(cash_flow.amount.currency).to eq currency
    end
  end
end

Model:

class CashFlow < ActiveRecord::Base
  belongs_to :user
  has_and_belongs_to_many :tags

  monetize :amount_cents, with_model_currency: :amount_currency

  validates_presence_of :user, :target_date, :amount_cents, :amount_currency
  validates_exclusion_of :amount_cents, in: [0, ]

  scope :incoming, -> { where 'amount_cents > 0' }
  scope :outcoming, -> { where 'amount_cents < 0' }

  scope :past, lambda { |date = nil | where 'target_date < ?', date || Date.today}
  scope :future, lambda { |date = nil | where 'target_date > ?', date || Date.today}
end

Factory:

FactoryGirl.define do
  factory :cash_flow do
    association :user

    ignore do
      negative false
      past false
      threshold nil
      currency 'USD'
    end

    after :build do |cash_flow, evaluator|
      sign = evaluator.negative ? -1 : 1
      cash_flow.amount = Money.new((rand(10000) + 1) * sign, evaluator.currency || currency)

      sign = evaluator.past ? -1 : 1
      cash_flow.target_date = (evaluator.threshold || Date.today) + ((rand(60 * 24 * 30) + 1) * sign).minutes
    end
  end
end

What can you tell me about its readability, flexibility and coverage fullness?

I use factory girl, shoulda and money gems.

First thing: congratulations on starting with rspec! Way to go.

My first remark would be to remove your three first contexts (Fields, Associations & Validations).

Why? Because those are not testing any parts of your code - they are testing ActiveModel functionalities. You want to test your code, not your librairies - at least, not without a very good reason to think that there is a problem there. This is time spent to write specs that do not provide any benefits.

On the other side, while your model don't have much logic yet, I like the fact that you tested your scopes - this is your logic, and it should be tested.


SHA256 hash bruteforcer I made a SHA256 hash bruteforcer as my first program in Rust. It's very specific to what I'm trying to bruteforce and I'll probably never find it.

I translated it from Python to make it faster, but can I make it even faster than this?

// Disable snake case warning
#![allow(non_snake_case)]

extern crate crypto;
extern crate rand;

use self::crypto::digest::Digest;
use self::crypto::sha2::Sha256;
use std::{i64, fs, process};
use std::time::Instant;
use rand::Rng;

fn main() {
   // Local variables
   let listWin = ["black","green","orange","orange","orange","black","black","orange","black","black","black","orange","black","black","orange","orange","green","black","orange","black","black","orange","orange","black","black","black","orange","black","black","orange","orange","orange","orange","black","orange","black","black","orange","orange","black","black","orange","green","black","orange","orange","orange","orange","green","black"];
   let public_seed = "2132110516";
   let round_ = 4645230;

   let now = Instant::now();
   let mut nbOfTry: u32 = 0;
   let mut roundVerif = 0;
   let mut predict;

   loop {
      let randomSeed = &CreateSeed(32);
      predict = PredictColor(randomSeed, public_seed, round_);

      // Check if the hash is correct by predicting the first 50 round of the day
      while predict == listWin[roundVerif] {
         roundVerif += 1;
         predict = PredictColor(randomSeed, public_seed, round_ + roundVerif as u32);

         if roundVerif > 45 {
            fs::write("seed.txt", randomSeed).expect("Unable to write file");
            process::exit(1);
         }
      }
      roundVerif = 0;

      nbOfTry += 1;
      if nbOfTry % 1000000 == 0 {
         println!("{}", nbOfTry);
         println!("In {} secondes", now.elapsed().as_secs());
      }
   }
}

//---------------------------------------------------
// CreateSeed()
//---------------------------------------------------
// Description : Creates a random seed
//
// Input      : Number of byte
// Output     : Random seed with a len of 2*nbByte
//---------------------------------------------------
fn CreateSeed(nbByte: u8) -> (String){
   let mut rng = rand::thread_rng();
   let mut seed = String::new();
   let mut byte: String;

   for _x in 0..nbByte {
      let rand = rng.gen::<u8>();
      byte = format!("{:x}", rand);

      if rand <= 15 {byte = format!("0{}", byte);}
      seed = format!("{}{}", seed, byte);
   }
   return seed;
}

//---------------------------------------------------
// PredictColor()
//---------------------------------------------------
// Description : Predict the roll
//
// Input      : server_seed : serveur seed
//              public_seed : public seed
//              round_      : First round of the day
// Output     : --
//---------------------------------------------------
fn PredictColor(server_seed: &str, public_seed: &str, round_: u32) -> (String){
   let toHash;
   let mut hasher = Sha256::new();

   // Hash everything
   toHash = format!("{}-{}-{}",server_seed, public_seed, round_ );
   hasher.input_str(&toHash[..]);
   let hash_ = hasher.result_str();

   // Get the roll number from the hash
   let decNumber = i64::from_str_radix(&hash_[..8], 16);
   let roll = decNumber.unwrap() % 15;

   // Print the color
   if roll == 0 {return "green".to_owned();}
   else if roll >= 1 && roll <= 7 {return "orange".to_owned();}
   else if roll >= 8 && roll <= 14 {return "black".to_owned();}
   else{return "failed".to_owned();}
}

Run and address feedback from automated tools to get easy feedback without having to make use of another human's time. Possible tools include:

The Rust compiler. Don't turn off warnings.
Rustfmt is a tool for automatically formatting Rust code to the community-accepted style.
Clippy is a tool for finding common mistakes that may not be compilation errors but are unlikely to be what the programmer intended.

These detect things like:

incorrect / inconsistent indentation.
unneeded usages of return
non-idiomatic Rust naming

Your variables have useless prefixes and suffixes. Don't blindly use Hungarian notation in a strongly typed language (e.g. nb). Dnt ndlssly abbv vrbs (don't needlessly abbreviate variables).

With this initial clearing of the underbrush, we can start to look at the code.

Don't use extern crate in modern Rust (What's the difference between use and extern?).
Don't use self::cratename, just use cratename.
I prefer to group all imports from the same crate into a single use statement.
Declare your variables as close as possible to where they are defined. Declare them inside of loops, especially if they aren't used outside of the loop or are reset inside the loop.
Use an infinite range instead of manually incrementing a variable.
Your function documentation is mostly useless. It's not using a doc comment (///). It repeats things like the function name. (How do you document function arguments?)
The type (String) has an unneeded parenthesis.
Your number formatting is grossly inefficient. You allocate many strings when you only need to extend an existing one. (What is the easiest way to pad a string with 0 to the left?; How can I append a formatted string to an existing String?)
Don't use _x if the variable is never used. Just use _.
Don't use magical values like "failed". Make use of Rust's Result type (What's the benefit of using a Result?)
There's no need to slice the string with [..]. Most of the time, a reference to a String will suffice (How does a reference to a String become a string slice?)
There's no need to return an owned String; a &'static str will suffice (What are the differences between Rust's String and str?)
Avoid using as to convert numbers. Prefer From::from instead (How do I convert between numeric types safely and idiomatically?)
It's inefficient to convert the digest result to a string, slice the string, then convert back to a number. Perform the operations directly on the digest buffer. (How can I convert a buffer of a slice of bytes (&[u8]) to an integer?; How to get a slice as an array in Rust?)
Don't use an i64 when you mean to use an u32
Using a match is cleaner than multiple if/else statements.

use crypto::{digest::Digest, sha2::Sha256};
use rand::Rng;
use std::{convert::TryInto, fs, process, time::Instant};

fn main() {
    // Local variables
    let winners = [
        "black", "green", "orange", "orange", "orange", "black", "black", "orange", "black",
        "black", "black", "orange", "black", "black", "orange", "orange", "green", "black",
        "orange", "black", "black", "orange", "orange", "black", "black", "black", "orange",
        "black", "black", "orange", "orange", "orange", "orange", "black", "orange", "black",
        "black", "orange", "orange", "black", "black", "orange", "green", "black", "orange",
        "orange", "orange", "orange", "green", "black",
    ];
    let public_seed = "2132110516";
    let round = 4_645_230;

    let now = Instant::now();

    for tries in 0.. {
        let random_seed = &create_seed(32);
        let mut predict = predict_color(random_seed, public_seed, round).unwrap();
        let mut round_verifications = 0u8;

        // Check if the hash is correct by predicting the first 50 round of the day
        while predict == winners[usize::from(round_verifications)] {
            round_verifications += 1;
            predict = predict_color(random_seed, public_seed, round + u32::from(round_verifications))
                .unwrap();

            if round_verifications > 45 {
                fs::write("seed.txt", random_seed).expect("Unable to write file");
                process::exit(1);
            }
        }

        if tries % 1_000_000 == 0 {
            println!("{}", tries);
            println!("In {} seconds", now.elapsed().as_secs());

            if tries == 5000000 { return }
        }
    }
}

/// Creates a random seed with a length of `2 * bytes`
fn create_seed(bytes: usize) -> String {
    let mut rng = rand::thread_rng();
    let mut seed = String::with_capacity(2 * bytes);

    for _ in 0..bytes {
        use std::fmt::Write;
        write!(&mut seed, "{:02x}", rng.gen::<u8>()).unwrap();
    }
    seed
}

/// Predict the roll
fn predict_color(
    server_seed: &str,
    public_seed: &str,
    first_round_of_the_day: u32,
) -> Result<&'static str, ()> {
    let mut hasher = Sha256::new();

    // Hash everything
    let to_hash = format!("{}-{}-{}", server_seed, public_seed, first_round_of_the_day);
    hasher.input_str(&to_hash);

    let mut hash = [0; 32];
    hasher.result(&mut hash);

    let leading_bytes: &[u8; 4] = hash[..4].try_into().unwrap();
    let number = u32::from_be_bytes(*leading_bytes);

    match number % 15 {
        0 => Ok("green"),
        1..=7 => Ok("orange"),
        8..=14 => Ok("black"),
        _ => Err(()),
    }
}

Running this in release mode:

% time ./target/release/review
0
In 0 seconds
1000000
In 2 seconds
2000000
In 4 seconds
3000000
In 7 seconds
4000000
In 9 seconds
5000000
In 11 seconds

real    12.078  12077581us
user    11.879  11879005us
sys 0.033   33467us
cpu 98%
mem 968 KiB

Compared to your original:

% time ./target/release/review
1000000
In 14 secondes
2000000
In 28 secondes
3000000
In 42 secondes
4000000
In 56 secondes
5000000
In 70 secondes

real    1:10.49 70485131us
user    1:10.16 70162635us
sys 0.137   136869us
cpu 99%
mem 980 KiB

The improved version takes 17% of your original time (a 5.8x speed increase).

Using a random number generator for the seed is probably a bad idea. It's totally reasonable for a RNG to repeat a value, but that won't change your results. You might as well start at 0 and work your way up. This also makes it trivial to stop and resume.

The problem appears to be embarrassingly parallel, so that would also be an avenue to investigate.


Fetching the website version number from the database I was just writing some code the other day, and I found myself writing something I am not sure whether it is OK to do it like this, or not.

Let me give an example of what this is:

I have this Method:

    public static string STOLENVersion()
    {
        string version = string.Empty;
        try
        {
            StringBuilder sb = new StringBuilder();
            sb.AppendLine("SELECT TOP 1");
            sb.AppendLine("    [WebsiteVersion]");
            sb.AppendLine("FROM [WebsiteDefaults]");
            using (SqlCommand cmd = new SqlCommand(sb.ToString(), NewSQLConnection))
            {
                cmd.Connection.Open();
                version = cmd.ExecuteScalar().ToString();

                cmd.Connection.Dispose();
            }
        }
        catch (Exception)
        {
            throw;
        }
        return "Version: " + version;
    }

And I have this property:

public static SqlConnection NewSQLConnection
    {
        get
        {
            return new SqlConnection(ConfigurationManager.ConnectionStrings["DebugDB"].ConnectionString);
        }
    }

I am disposing the SqlCommand's connection inside its using block, and this method can be called multiple times (maybe like 10 times every minute, as users log on to the website, as an example)

The try-catch-throw with no error handling is completely pointless. I would just remove it and let the exception bubble up the call stack.
There is no need to call connection.Dispose() since the using () { ... } block will do that automatically
A StringBuilder object is like hitting a thumbtack with a sledge hammer in this case. You can prefix a string literal with the @ symbol to allow newline characters for better readibility:

string sql = @"SELECT TOP 1
               [WebsiteVersion]
               FROM [WebsiteDefaults]";

Or since this is such a simple SQL statement, a one-liner is appropriate as well:

string sql = "SELECT TOP 1 [WebsiteVersion] FROM [WebsiteDefaults]";