Single producer single consumer command queue I needed to create commands simply with lambda and execute it in another thread. The following code works fine. Any suggestions about performance are appreciated.

I want to use SSE aligned variables as lambda captures, for instance:

__m128 xmm0;
queue.Enqueue([xmm0](){
    //...
});

template<unsigned MaxCommand = 1000> class TCommandQueueSPSC
{
    static const unsigned MaxLambdaSize = sizeof(void*) * 7;

    struct alignas(16) ICMD
    {
        virtual ~ICMD() {}
    };
    struct alignas(16) CMDBase : ICMD
    {
        char mBuffer[MaxLambdaSize];
    };
    static_assert(sizeof(CMDBase) % 16 == 0, "should be 16 aligned");
    static const unsigned CMDSize = sizeof(CMDBase);

    alignas(64) volatile unsigned       mHead = 0;
    volatile bool                       mRuning = false;
    Event                               mWaitEvent;
    alignas(64) volatile unsigned       mTail = 0;
    ThreadID                            mConsumerThreadID = 0;
    alignas(64) CMDBase                 mElements[MaxCommand];

    bool IsEmpty() const { return mHead == mTail; }
    bool IsFull() const { return ((mTail + 1) % MaxCommand) == mHead; }

    void* BeginEnqueue()
    {
        UASSERT(!IsFull()); //assert if full
        return mElements + mTail;
    }
    void EndEnqueue()
    {
        mTail = (mTail + 1) % MaxCommand;
    }

    ICMD* BeginDequeue()
    {
        if (mHead == mTail) return nullptr;
        return (ICMD*)(mElements + mHead);
    }
    void EndDequeue()
    {
        mHead = (mHead + 1) % MaxCommand;
    }

public:
    TCommandQueueSPSC(const TCommandQueueSPSC&) = delete;
    TCommandQueueSPSC& operator = (const TCommandQueueSPSC&) = delete;

    TCommandQueueSPSC() : mWaitEvent(false, false) {}

    template<typename Lambda> void Enqueue(const Lambda& proc)
    {
        if (Thread::ID() == mConsumerThreadID)
        {
            proc();
            return;
        }

        struct NewCMD : public ICMD
        {
            Lambda mProc;
            NewCMD(const Lambda& p) : mProc(p) {}
            ~NewCMD()
            {
                mProc();
            }
        };
        static_assert(sizeof(NewCMD) <= CMDSize, "");
        static_assert(alignof(NewCMD) <= alignof(ICMD), "");
        void* cmd = BeginEnqueue();
        new (cmd) NewCMD(proc);
        EndEnqueue();
    }
    template<typename Lambda> void EnqueueAndWait(const Lambda& proc)
    {
        if (Thread::ID() == mConsumerThreadID)
        {
            proc();
            return;
        }

        struct NewCMD : public ICMD
        {
            Lambda mProc;
            TCommandQueueSPSC*  mOwner;
            NewCMD(const Lambda& p, TCommandQueueSPSC* owner) : mProc(p), mOwner(owner) {}
            ~NewCMD()
            {
                mProc();
                mOwner->mWaitEvent.SetSignaled();
            }
        };
        static_assert(sizeof(NewCMD) <= CMDSize, "");
        static_assert(alignof(NewCMD) <= alignof(ICMD), "");
        void* cmd = BeginEnqueue();
        new (cmd) NewCMD(proc, this);
        EndEnqueue();

        mWaitEvent.Wait();
    }
    void RunTillQuit()
    {
        mRuning = true;
        mConsumerThreadID = Thread::ID();

        while (mRuning)
        {

            if (ICMD* cmd = BeginDequeue())
            {
                cmd->~ICMD();
                EndDequeue();
            }
            else
            {
                 _mm_pause();
            }
        }
    }
    void Quit() { mRuning = false; }
    volatile bool IsRuning() const { return mRuning; }
 };

Order of public, protected, private

This is a personal preference but one that I find makes code easier to read. I always order my fields as: public then protected then private with the motivation that the person using the class is interested in seeing the public API and this should thus be the first thing you see so that you don't have to search for it.

Misuse of volatile

C++ (and C) has a thing called the "as-if rule". Which in essence says:

  The compiler is allowed to generate ANY code as long as all reads and writes to volatile memory happens in the same order and with the same values as if the program was execute according to the language specification.

This is the only use volatile has in C++, it means that writes and writes to volatile memory will always happen and will happen in the order specified. For example the compiler can, and frequently does re-order stores and writes. The compiler can remove memory writes and reads if it can determine that they will not affect any volatile memory read or write. The compiler can even remove memory allocations if it so pleases (clang does this some times).

For completeness, all inputs and outputs (files, std::cout, std::cin, std::cerr, std::clog, keyboard drivers, graphics display, sound buffers, networks packets) are volatile either directly or transitively. So the compiler cannot remove inputs or outputs of the program, but anything in-between basically.

When should I use volatile then?

Volatile should be used when you must be certain that a certain write or read is not removed by the compiler as dead. For example when you are writing a hardware driver, you would set control variables that must be written to some bus as volatile.

But volatile works for synchronising my threads just fine!

That is pure luck that it is working on your compiler. The compiler is allowed to change the order of reads and writes AROUND your volatile accesses as long as they don't affect the values of the volatile accesses. Also the CPU may or may not re-order some stores or loads depending on the CPU (x86 has a conveniently strict memory model which only has one case where memory accesses may complete out of order).

What you need is a memory barrier. A memory barrier forces both the compiler to generate code in such a way that all writes before the barrier happens before the barrier and tells the CPU to make sure all writes before the barrier has happened before the barrier completes. The above is a bit simplified, there are different memory types of barriers which say what must be completed before the barrier completes. If you're interested, see std::memory_order.

Does volatile affect my performance?

Yes, the compiler can do quite some fancy optimisations with re-ordering stores and loads and instructions so that they will effectively use the CPU's registers and to make sure that multiple issue kicks in on some CPUs. By using volatile you are prohibiting the compiler from doing some of these optimisations as you are forcing it to emit a read or write in a specific order.

In summary, don't use volatile unless you're absolutely sure that you need it.

Over-aligning basic types

You specify:

alignas(64) volatile unsigned       mHead = 0;
volatile bool                       mRuning = false;
Event                               mWaitEvent;
alignas(64) volatile unsigned       mTail = 0;
ThreadID                            mConsumerThreadID = 0;
alignas(64) CMDBase                 mElements[MaxCommand];

The compiler is required to store members of a struct or class in the order they are specified in the definition. And it is also required to respect the requested alignment. So the above code will form a structure in memory that looks something like this (depending on platform, unsigned is at least 16 bits but most often 32 bits):

Byte offset  Value
0-3         mHead
4           mRunning
Padding to respect basic alignment of mWaitEvent which believe is 4
8-?         mWaitEvent (I don't know how big it is)
Padding to make sure mTail starts on 64 byte boundary.
64-67       mTail
68-71       mConsumerThreadID (should be 32 bits)
72-127 Padding to make sure mElements start on a 64 byte boundary.
128 - 64127  mElements (assuming 64 bit binary)

Note that as CMDBase has `alignas(16)` and the structure is `MaxLamdaSize = 8*7 = 56 bytes` it has to be rounded up to the nearest multiple of 16 which is 64, so each CMDBase will occupy 64 bytes in that array.

Compare the above to the following:

unsigned       mHead = 0;
unsigned       mTail = 0;
ThreadID       mConsumerThreadID = 0;
Event          mWaitEvent;
bool           mRuning = false;
CMDBase        mElements[MaxCommand];

Byte offset  Value
0-3         mHead
4-7         mTail
8-13        mConsumerThreadID
14-21(?)    mWaitEvent (I don't know how big it is, assuming 8 bytes now)
22          mRuning
32 - 6431   mElements (assuming 64 bit binary)

Notice how the struct is 90 bytes shorter just by re-arranging the order of the members and removing the useless padding. Looking at it from a relative point of view 90 bytes out of 64127 is less than 0.2% storage reduction but that is not the point. Typically CPUs have a cache line that is 64 bytes long, i.e. the CPU can cache data in batches of 64 bytes. In this later variation all of the control variables fit in one cache line, where in your original code you needed two cache lines to hold the control variables, leaving less cache lines for the rest of the data.

Further more the alignment of data doesn't affect performance in any way as long as the data is aligned so that the CPU doesn't have to do an unaligned load (as long as you don't get an unaligned load but standard alignment requirements in C++ prevent this automatically).

Alignment is only useful to guarantee that hardware which requires a certain alignment can function and when you want to avoid false sharing. For example SSE instructions require 16 byte alignment on their loads. Don't just go randomly sprinkling alignas in your code. It will HURT your performance.

Enough about conceptual misunderstandings and on to the code

So it looks like you have implemented a circular buffer.  I recommend that you break out the circular buffer into it's own class and then use that in implementing your command queue. This will make your code clearer and easier to read. It will also make it possible to re-use the circular buffer.

Don't use the destructor to run the code like you do here:

       ~NewCMD()
        {
            mProc();
        }

If the lamda throws then you might have problems on your hands as destructors throwing exceptions is kind of a bad situation throwing from destructor. Not to mention that destructors are only for destroying the object, releasing any resources held and cleaning up.

Further since you're not removing the commands from the queue, when your application shuts down the mElements array will be destructed and each of the destructors on the elements will be executed again. Do I need to say this is bad? Not to mention that you never remove the objects from the queue when you're done with them.

I feel that you are overworking this in an effort to make it fast but in practice you're introducing problems and not actually improving speed. If you want your lamda to be 16 byte aligned so that your captures will work then just say so.

alignas(16) struct AlignedFunction{
    std::function<void(void)> function;
};

std::queue<AlignedFunction> mElements;

template<typename Callable>
void enqueue(Callable&& callable){
    mElements.emplace_back([](){callable();});
}

Don't preemptively optimise

Have you measured your application and have hard evidence that this command queue is the bottleneck of your application? If not you're just wasting time and creating bugs trying to write this complex class. I recommend that you take a simpler approach using standard library algorithms and functions and model the behaviour you want. If at a later point you get profiling data that points out this class as a bottle neck, then you can optimise.

In summary

I see that you are trying a lot of tricks that you think will improve performance. In reality these do no improve your performance, in fact I'd be willing to wager that you are in fact hurting your performance instead. Not only that but they are making the code hard to read and hard to work with.

Stop trying to be clever, you are hurting yourself. Write clean, maintainable code instead. The compilers today are pretty darn good when it comes to generating good code. Most likely they are way better than you are.


Leetcode Search in Rotated Sorted Array I was doing Search in sorted array question from leetcode
Question
Suppose an array sorted in ascending order is rotated at some pivot unknown to you beforehand.
(i.e., [0,1,2,4,5,6,7] might become [4,5,6,7,0,1,2]).
You are given a target value to search. If found in the array return its index, otherwise return -1.
You may assume no duplicate exists in the array.
Your algorithm's runtime complexity must be in the order of O(log n).
Example
Input: nums = [4,5,6,7,0,1,2], target = 0
Output: 4
My solution
/**
 * @param {number[]} nums
 * @param {number} target
 * @return {number}
 */
var search = function(nums, target) {
    for (let i=0; i<nums.length; i++) {
        if (target === nums[i]) return i
    }
    return -1
};

While this algo works, I am not sure if this is optimal or correct? for question with medium difficulty. Can someone verify it?

I start from your code:

var search = function(nums, target) {
    for (let i=0; i<nums.length; i++) {
        if (target === nums[i]) return i
    }
    return -1
};

And your question:

  While this algo works, I am not sure if this is optimal or correct?

Your algorithm is correct but it is optimal ? The answer is nope because as from the comment by @CertainPerformance to your question your code runs in O(n).

The hint is contained in the phrase Your algorithm's runtime complexity must be in the order of O(log n) that bounds the desired solution to the binary search having O(log n) complexity. The algorithm works if there is a sorted array, but with some modification can be adapted to a rotated array.

Here my javascript code for the original binary search:

const search = function search(a, t) {
    let l = 0;
    let r = a.length - 1;

    while (l <= r) {
        let m = Math.floor((l+r) /2);
        if (a[m] == t) { return m;  }
        if (a[m]  < t) { l = m + 1; }
        if (a[m]  > t) { r = m - 1; }
   }
   return -1;
}

My solution for a rotated array is quite similar but with a two if else inside the while cycle because we can have four different cases, I added an explanation after the code below:

const search = function search(a, t) {
    let l = 0;
    let r = a.length - 1;

    while (l <= r) {
        let m = Math.floor((l+r) /2);
        if (a[m] == t) { return m; }

        if (a[m] < a[r]) {
            if (t > a[m] && t <= a[r]) {
                l = m + 1;
            } else {
                r = m - 1;
            }
        } else {
            if (t >= a[l] && t < a[m]) {
                r = m - 1;
            }
            else {
                l = m + 1;
            }
        }
    }

    return -1;
}

console.log(search([4,5,6,7,0,1,2], 0));
console.log(search([4,5,6,7,0,1,2], 5));
console.log(search([4,5,6,7,0,1,2], 3));
console.log(search([1, 3], 3));
console.log(search([4,5,6,7,8,1,2,3], 8));

Substantially if I have a [l,..m, .. r] interval I have two possible cases:

a[m] < a[r] --> consecutive ascending numbers, so if a[m] < t <=
a[r] in the next iteration I will examine the interval [m + 1, r]
otherwise I will examine the interval [l, r - 1]
a[m] >= a[r] --> there are rotated elements like for example [7, 0,
1, 2] , so I am sure the left array contains only ascending numbers.
In this case if a[l] <= a[t] < a[m] in the next iteration I will
examine the interval [l, m - 1] otherwise I will examine the
interval [m + 1, r].


Clearing and loading values on HTML form fields I have a bootstrap modal which contains a form that I'm using to add a new record and update existing ones.

I have a button that runs a method (showReferenceModal()) for clearing the field before showing the modal. And another button that runs a method (loadReferencesDetails(referenceId)) for loading the details before showing the modal.

function showReferenceModal() {
    ...
    $('#referenceId').val("");
    $('#name').val("");
    $('#addr').val("");
    $('#tel').val("");
    $('#updateInfo').html("");
    $('#modal').modal('show');
}

function loadReferencesDetails(referenceId) {
    ...
    $.ajax({
        ...
        dataType: 'json'
    }).done(function(response) {
        $('#referenceId').val(response.referenceId);
        $('#name').val(response.name);
        $('#addr').val(response.addr);
        $('#tel').val(response.tel);
        var updateInfo = "";
        ...
        $('#updateInfo').html(updateInfo);
        $('#modal').modal('show');
    });
}

Possible Improvements:

Is there a better way to clear the fields all at once? Because when the form gets bigger, then the method to show the modal will be bigger too. It's okay for now, but I'm thinking about extensibility. I'm new to jQuery so I don't really know if there's a method to do this.
For loading the details on the form. As you can see, the response from my Servlet returns a JSON with the same property name as the ID of the FIELD it belongs to. Maybe there's a shortcut for this too?
By the way, I have multiple modals, which have their own set of these two methods. Any tip so I can just use a single method for different modals instead of creating their own showXXXModal() and     loadXXXDetails()?

Good question,

you can clear all (visible) input elements like this:

$("input:text").is(":visible").val('');

In general, google queries are a gold mine here. Just google 'jQuery clear all text fields' and you will find a link to StackOverflow with several approaches.

You are correct that there are easier ways to get your response in the correct fields, something like this might work for you:

}).done(function(response) {
    Object.keys(response).forEach( function( field ){
      $('#'+field).val(response[field]);
    });
    var updateInfo = "";
    ...
    $('#updateInfo').html(updateInfo);
    $('#modal').modal('show');
});

As for #3 you should look into libraries to that work for you. A number of folks have done all the heavy lifting for that already.


Send / receive wrapper for System.Net.Sockets - follow-up I recently posted a question about improving thread and socket safety in my NetworkEndpoint class. previous post

I have implemented the changes suggested in the answer I got there. When I asked for a second look to confirm my changes I was told to post another question.

Included below is the code I changed as well as a few follow up questions. To see the entire class please refer to my previous post.

Is my use of locks in the Connected property and Send and Disconnect methods effective?

private object connectedLock = new object();
public bool Connected {
    get {
        lock (connectedLock) {
           if (connection == null) return false;
           return connection.Connected;
        }
    }
}

private object sendLock = new object();
public void Send(NetworkHeader header, byte[] data) {
    if (!Connected) throw new InvalidOperationException("NetworkEndpoint must be connected before sending.");
    try {
        lock (sendLock) {
            connection.Send(ByteUtils.Combine(header.Serialize(), data));
        }

    } catch (SocketException) {
        Disconnect();
    }
}

private object disconnectLock = new object();
public void Disconnect() {
   if (Connected) {
        lock (disconnectLock) {
            connection?.Shutdown(SocketShutdown.Both);
            connection?.Close();
            connection = null;
            OnDisconnected();
            Clear();
       }
    }
}

Is the lock I added to InitializeReceiveLoop redundant due to if (Receiving) return;?

private object initializeLock = new object();
public void InitializeReceiveLoop() {
    lock (initializeLock) {
        if (Receiving) return;
        Receiving = true;
    }
    BeginReceive();
}

Is this the best way to clear events? Would setting the events to null achieve the same goal?

public void Clear() {
    foreach (Delegate d in DataReceived.GetInvocationList())
        DataReceived -= (EventHandler<NetworkReceiveEventArgs>)d;
    foreach (Delegate d in Disconnected.GetInvocationList())
        Disconnected -= (EventHandler)d;
}

Is this implementation of EndReceive better than my previous?
Is EndReceive not already "thread-safe" by nature of how it is used? It is a private method only ever called by BeginReceive which is only called after the completion of a previous call to EndReceive or by InitializeReceiveLoop which can only be called once (returns imediately if Receiving is already true)

private void EndReceive(IAsyncResult result) {
    byte[] dataBuffer = null;
    NetworkHeader header = null;

    try {
        if (connection.EndReceive(result) > 0) {
            header = NetworkHeader.Deserialize(headBuffer);
            dataBuffer = new byte[header.DataSize];

            int offset = 0;
            while (offset < header.DataSize) {
                int lengthRead = connection.Receive(dataBuffer, offset,
                    (int)header.DataSize - offset, SocketFlags.None);

                if (lengthRead == 0) {
                    Disconnect();
                    return;
                }

                else offset += lengthRead;
            }
        }
    } catch (SocketException) {
        Disconnect();
        return;
    }

    OnDataReceived(header, dataBuffer);
    BeginReceive();
}

Events can be cleared much simpler:

public void Clear() {
    foreach (Delegate d in DataReceived.GetInvocationList())
        DataReceived -= (EventHandler<NetworkReceiveEventArgs>)d;
    foreach (Delegate d in Disconnected.GetInvocationList())
        Disconnected -= (EventHandler)d;
}

public void Clear() {
    DataReceived = null;
    Disconnected = null;
}

This is by no means a full review of your code, but I would like to explain a couple of things to you.

You are using seperate locks for sending, checking connection status, initializing and disconnecting. Think about the implications by using a seperate lock for operations that all require a common thread-aware resource Connected. Since they all take a different lock, they can simultaniously change the state and corrupt the other ongoing operation. This is why you should use a single lock for all these operations.

private object syncRoot = new object();

But even then, since you check the condition before the lock, data integrity might still be breached by an unfortunate flow of events.

public void Send(NetworkHeader header, byte[] data) {
    if (!Connected) // ..      // <- outside lock
    lock (syncRoot) {          // <- inside lock, but condition might no longer be true
        // ..
    }
}

As an example, suppose two threads simultaniously call respectively Send and Disconnect. A possible flow is:

- thread A: call Send                    // thread A is first
- thread A: check IsConnected: true
- thread B: call Disconnect              // thread B starts a fraction later
- thread B: check IsConnected: true
- thread B: take lock
- thread B: set IsConnected: false
- thread B: release lock
- thread A: take lock                    // thread A reaches a point where it
             // expected its state to still be valid, but thread B changed it
- thread A: connection.Send(..)          // <- null-reference exception

There used to be a recommedation about double-checked locking. But here is a good reason not to use it. So we will have to take the condition inside the lock.

public void Send(NetworkHeader header, byte[] data) {
    lock (syncRoot) {
        if (!Connected) // ..
        // ..
    }
}

Moving on, we can change Connected to take a volatile boolean, so we don't require additional locking when just getting its value. Just make sure to set its value when connection is established and reset on Disconnect. The volatile field is read as an atomic operation. You get the actual value, rather than a cached value (which might be done to optimize state in a non-concurrent context).

private volatile bool _connected = false;

public bool Connected => _connected;   // <- no locking required

I hope this explains some basic concepts of locks and thread-safety.


Searching repositories for files with forbidden strings I wrote a small tool for searching git repositories for forbidden strings that I don't want to commit publicly. It's still something basic but it does the job.

The project contains three files.

config.py - this is the file where I store the secrects like which directories to scan and which strings to look for in a long regex (here redacted). This is commited to a VSTS repository because they are private.

import re

paths = [
        "c:\\..",
        "c:\\.."
    ]

re_forbidden = re.compile(r"(class)")

main.py - this is the core file. It uses os.walk to list all files and directories. Not all directories and files are processed. Paths that don't get commited like .git or bin and files like dll are skipped. Paths and the re_forbitten regex are imported from the above config.py. When found something suspicious then it just outputs the path of the affected file.

import os
import time
import itertools
import shutil
import re
import importlib.util
config_spec = importlib.util.spec_from_file_location("config", "c:\\home\\projects\\classified\\python\\sanipyzer\\config.py")
config = importlib.util.module_from_spec(config_spec)
config_spec.loader.exec_module(config)
from pprint import pprint

ignore_dirs = [".git", "bin", "obj", ".vs"]
ignore_files = ["dll", "exe", "pdb", "map"]

def format_filemtime(path):
    filemtime = os.path.getmtime(path)
    return time.strftime('%Y-%m-%d', time.gmtime(filemtime))

def ignore_dir(dirpath):
    for dir in ignore_dirs:
        pattern = r"\\" + re.escape(dir) + r"(\\|$)"
        if re.search(pattern, dirpath):
            return True
    return False

def ignore_file(file_name):
    for ext in ignore_files:
        pattern = r"\." + ext + "$"
        if re.search(pattern, file_name):
            return True
    return False

def sanitize(path):

    start = time.perf_counter()

    for (dirpath, dirnames, filenames) in os.walk(path):
        if ignore_dir(dirpath):
            continue
        searchable_filenames = [filename for filename in filenames if not ignore_file(filename)]
        for filename in searchable_filenames:
            full_name = os.path.join(dirpath, filename)

            # without 'ignore' it throws for some files the UnicodeDecodeError 'utf-8' codec can't decode byte XXX in position XXX: invalid start byte
            with open(full_name, 'r', encoding="utf8", errors="ignore") as searchable:
                text = searchable.read()
                if config.re_forbidden.search(text):
                    pprint(full_name)

    end = time.perf_counter()
    elapsed = round(end - start,2)
    print(f"elapsed: {elapsed} sec")

# --- --- ---

def main():
    for path in config.paths:
        sanitize(path)

if __name__ == '__main__':
    main()

test.py - this file is a very simple unittest. It tests the two ignore methods.

import unittest

from main import ignore_dir
from main import ignore_file

class MainTest(unittest.TestCase):

    def test_ignore_dir(self):
        self.assertTrue(ignore_dir("c:\\temp\\.git"))
        self.assertTrue(ignore_dir("c:\\temp\\.git\\abc"))

    def test_ignore_file(self):
        self.assertTrue(ignore_file("c:\\temp\\project\\program.dll"))
        self.assertFalse(ignore_file("c:\\temp\\project\\program.cs"))
        # tests the test
        #self.assertTrue(ignore_file("c:\\temp\\project\\program.cs"))

if __name__ == "__main__":
    # use False to avoid the traceback "Exception has occurred: SystemExit"
    unittest.main(exit=False)

What do you think of it? Is this an acceptable solution or are there still many beginner mistakes? I wasn't sure about the two ignore lists whether they are const and should be UPPER_CASE or are they just simple variables?

In general, it is a good code. However, I have a few remarks. Note: I am assuming that the code works perfectly as it should and will comment only on the style. Also, I assume that you are an experienced programmer and hence will not be explaining the basics, although if something is not clear please let me know.

Configuration

You might want to keep the real paths separate to a configuration script. I'd advice you to create a *.cfg file that would store all the configuration data. I can imagine this being used on different machines with different paths each. I don't think you would like to allow users to modify the code, it is better to give them config file.

In case you would like to keep the configuration as it is, I'd encourage to move the lists to config.py and name (as you mentioned) in UPPER_CASE. Additionally, you might also want to encapsulate them in a functions like this:

def get_ignored_dirs():
    return ignored_dirs

This allows to keep the API of config.py constant regardless on what you decide to do with inners of the configuration.

Lastly, you might consider moving your functions like ignore_dir to separate file (e.g. files_utils.py) and treat is as a proxy to your configuration.

Method naming

In general I tend do explicitly say that some function returns boolean value by sticking is at the beginning. For example, it is clear (from the code) that your function ignore_dir checks if the directory should be ignored. This is however, not clear from the name, one might think that this function ignores some directory. I'd suggest to change the name to is_ignored_dir or something similar. The same goes for ignore_file.

Separation of concerns and responsibilities

It seems for me that your sanitize function is not very SRP-like. Inside, you do many things including deciding whether to ignore a directory (or file), read the file and count time. I'd suggest dividing those responsibilities to more than one function. You might notice that if you try to do a unit test for this function, in reality it will be a e2e test.

Below, an example of what you can fix it.

def measured(func):
    def wrapper(path):
        start = time.perf_counter()
        func(path)
        end = time.perf_counter()
        elapsed = round(end - start,2)
        print(f"elapsed: {elapsed} sec")
    return wrapper

def searchable_files_from_list(filenames):
    for filename in filenames:
        if not is_ignore_file(filename):
            yield filename

def searchable_files_containing_forbidden_text(filenames):
    for filename in searchable_files_from_list(filenames):
        full_name = os.path.join(dirpath, filename)
        if contains_forbidden_text(full_name):
            yield full_name

def contains_forbidden_text(filename):
    with open(full_name, 'r', encoding="utf8", errors="ignore") as searchable:
    text = searchable.read()
    if config.re_forbidden.search(text):
        return True
    return False

def files_to_sanitize(path):
    for (dirpath,dirname,filenames) in os.walk(path):
        if is_ignored_dir(dirpath):
            continue
        for file in searchable_files_containing_forbidden_text(filenames):
            yield file

@measured
def print_files_to_sanitize(path):
    for file in files_to_sanitize(path):
        pprint(file)

Explanation:

Now, all of your methods can be separately unit tested and validated. Let's take look at each function at a time, starting with the last one.

print_files_to_sanitize
This is what is left from your sanitize function. After the refactoring it become apparent that the function does not sanitize anything, it just prints a file name. This fact wasn't so clear before because of many things happening.

I've added a decorator to the function - this is a very common practice for measuring time of the method (you might want to generalize this by using *args and **kw).

files_to_sanitize is a generator that should give you all of the files that should be sanitized. It iterates through your catalogs ignoring some directories and getting the files from a second generator.

searchable_files_containing_forbidden_text Given a list of files it iterates over those which have a good file name (given by searchable_files_from_list function) and if such file contains a forbidden text it bubbles it up.


This program reads in integers until a sentinel number is read. And prints the largest integer read I'm just looking for feedback on how I could improve this simple program. Whether it be comments for better understanding or better variable names. I try not to focus on memory consumption because I'm honestly not that knowledgeable as of yet. My main focus is getting a written solution on paper and then implement in syntax.

I was given the question to write a program that reads in integers one per line, once a sentinel integer is read end program, and display the largest integer. The way I solved it was with pen and paper first because I'm working on my pseudocode and algorithm design. If there are any books on the subject for recommendation. I would greatly appreciate.

/**
 * This program reads in integers until a sentinel number is read,
 * and prints the largest.
 *
 * @author shawn
 *
 */

//Import needed for Console Programs.
import acm.program.*;

public class LargestListValue extends ConsoleProgram {
    public void run() {

        /*Integer variables to track the current largest input
         * and current input.
         */
        int x, y;

        println("This program reads in integers until a sentinel number is read, ");
        println("and prints the largest.");

        while (true) { //Initial while to read in an initial integer.
            x = readInt(" ? "); //Initial read integer. Also current largest.
            if (x == SENTINEL_NUMBER) {
                println("No numbers?");
                break;
            }
            else {
                y = readInt(" ? "); //Next read integer.
                if (y == SENTINEL_NUMBER) {
                    println("Largest number is " + x + ".");
                    break;
                } else {
                    while (y <= x) { //While second read integer
                        y = readInt(" ? ");
                        if (y == SENTINEL_NUMBER) {
                            println("Largest number is " + x + ".");
                            break;
                        } else if (y >= x) {
                            while (y >= x) {
                                x = readInt(" ? ");
                                if (x == SENTINEL_NUMBER) {
                                    println("Largest number is " + y + ".");
                                    break;
                                } else if (y <= x) break;
                            }
                        }
                    }
                }
            }
            //Extra break added to the
            break;
        }
    }
    //Sentinel number to end reading integers and print largest.
    private static final int SENTINEL_NUMBER = 0;
}

break;
            }
            else {

You don't need an else with a break.  The break ends execution inside the loop.  You can never reach the code after it in the loop unless it doesn't trigger.  Without all the else clauses, you can significantly reduce the amount of indentation.

Your code could also return instead of break.  That would be clearer in my opinion.

What's x?  What's y?  Why loop forever if you're just going to break out of it?

In my opinion, your comments would be better replaced with more readable, self-commenting code.

Consider the following

    public static int readLargest(final int SENTINEL) {
        int largest = readInt(" ? ");
        for (int current = largest; current != SENTINEL; current = readInt(" ? ") ) {
            if (current > largest) {
                largest = current;
            }
        }

        return largest;
    }

Which you'd use as

    public void run() {
        int largest = readLargest(SENTINEL_NUMBER);

        if (largest == SENTINEL_NUMBER) {
            println("No numbers?");
        } else {
            println("Largest number is " + largest + ".");
        }
    }

By using a separate method, we consolidate the possibilities.  We only have two println statements.  In the original code, you had the latter option three times.  Here we only need one.

This also makes the readLargest more reusable.  Even better might be to read all the values first, then call a findLargest method on that.  Then your readNumbers and findLargest methods would both be reusable separately.  Of course, you may not have gotten to Collection types yet.

In your original code, sometimes y was the largest and sometimes x.  In this code, largest is always the largest unless the most recent read returns a larger value.

I prefer the names current and largest as being more descriptive than single letter names.  I find that more important with largest.  I could live with x instead of current.

I'm not sure that it's necessary to say SENTINEL_NUMBER.  The current variable is an integer.  What would SENTINEL be?  A cabbage?  Obviously not.  You'd only compare an integer to some kind of number.  Others may disagree.  In particular, you may want to look up Hungarian Notation.

I prefer to pass the sentinel value as a parameter for flexibility.

Since we don't use current outside of the loop, I think that the for loop has better scope than a while loop.

The only comment that seems necessary is that readLargest will return the sentinel value if and only if there are no other numbers in the input (prior to the sentinel value).  Other than that, I feel that the code is pretty clear about what it is doing.  Perhaps some explanation of the initialization of largest.


Roman numeral kata in Elixir I'm new to Elixir, and in order to learn the syntax, I'm doing a roman numeral kata which converts a decimal number into roman numeral. I would appreciate any feedback you have.

defmodule RomanNumeral do

  @decimal_roman_numerals [[5, 'V'], [4, 'IV'], [1, 'I']]

  def converts(number) do
    converts(number, [], @decimal_roman_numerals)
  end

  def converts(number, roman_value, _) when number < 1 do
    roman_value
  end

  def converts(number, roman_value, [[decimal, roman] | numerals]) when number >= decimal do
    converts(number - decimal, roman_value ++ roman, [[decimal, roman] | numerals])
  end

  def converts(number, roman_value, [_ | numerals]) do
    converts(number, roman_value, numerals)
  end
end

Your code looks very nice indeed, I only have some minor observations you code use to improve this code:

Naming
Use the imperative form of the verb (convert instead of converts)

Implementation Hiding
Use defp for methods which are not intended for external use. You might consider also renaming them to start with underscore _ to further differentiate them as internal.

Multiple assignment in method signature
To make you code more succinct, you can use multiple assignment - meaning that instead of:

def converts(number, roman_value, [[decimal, roman] | numerals]) when number >= decimal do
  converts(number - decimal, roman_value ++ roman, [[decimal, roman] | numerals])
end

you can write:

def converts(number, roman_value, [[decimal, roman] | _] = numerals)
  when number >= decimal do
  converts(number - decimal, roman_value ++ roman, numerals)
end

to the same effect. This form assigns the first item in the list to [decimal, roman], as well as assigning the whole list (including the first element) to numerals, so you don't have to re-build it inside the method body.

Implementing the above suggestions, your code will look like this:

defmodule RomanNumeral do

  @decimal_roman_numerals [[5, 'V'], [4, 'IV'], [1, 'I']]

  def convert(number) do
    _convert(number, [], @decimal_roman_numerals)
  end

  defp _convert(number, roman_value, _) when number < 1 do
    roman_value
  end

  defp _convert(number, roman_value, [[decimal, roman] | _] = numerals)
    when number >= decimal do
    _convert(number - decimal, roman_value ++ roman, numerals)
  end

  defp _convert(number, roman_value, [_ | numerals]) do
    _convert(number, roman_value, numerals)
  end
end


Echo in prolog with single print In order to learn a bit more of Prolog, I implemented the echo command for SWI-Prolog.

I want a correct implementation, that is, it satisfies the following requirements:

It makes only one I/O call so that messages will not be garbled if multiple processes write to the same file.
If it is called without arguments, it just prints an empty line (newline).
It does not print any additional spaces. Only spaces between the arguments. No leading space. No trailing space.

Here's  the Prolog code that I came up with.

main(Argv) :-
    echo('', Argv).

echo(S, []) :-
    string_concat(S, '\n', S1),
    write(S1).
echo(S, [Last]) :-
    string_concat(S, Last, S1),
    echo(S1, []).
echo(S, [H|T]) :-
    string_concat(S, H, S1),
    string_concat(S1, ' ', S2),
    echo(S2, T).

I compile and test with the following Makefile (GNU make on Linux):

SHELL:=/bin/bash

.PHONY: all
all: echo

%: %.pl
    swipl -q -t main -o $@ -c $^

.PHONY: clean
clean::
    $(RM) echo

.PHONY: test
test:: echo
    diff (</bin/echo) <(./echo)
    diff (</bin/echo foo) <(./echo foo)
    diff (</bin/echo foo bar) <(./echo foo bar)

It can be run like this: ./echo foo bar.

Please consider using format/2 for formatting output.

You do not need so many concatenations!

Also, check out the following sample query and interaction:

?- main([test]).
test
true ;
test
true ;
false.

This shows that internally, there are choice-points left, and in fact even alternative solutions.

Ideally, the predicate should be deterministic, i.e., succeed exactly once.


Multiple loops versus multiple stringbuilders I've come across the following piece of code in a Razor view

        Company
        @foreach (Customer c in Model.SelectedInstallers)
        {
            @c.Name
        }

        Address
        @foreach (Customer c in Model.SelectedInstallers)
        {

                @c.Street

                @c.City

                @c.County

                @c.Postcode

        }

        Contact Details
        @foreach (Customer c in Model.SelectedInstallers)
        {

                @c.Telephone

                @c.Website

        }

        Remove
        @foreach (Customer c in Model.SelectedInstallers)
        {
            Remove installer
        }

Now would it better to stay with this code or change it to do one loop with multiple stringbuilders:

StringBuilder company = new StringBuilder();
StringBuilder address = new StringBuilder();
StringBuilder contact = new StringBuilder();
StringBuilder remove = new StringBuilder();

foreach (Customer customer in Model.SelectedInstallers)
{
    company.AppendFormat("\n    {0}	", customer.Name);
    address.AppendFormat("\n    {0}
{1}
{2}
{3}	", customer.Street, customer.City, customer.County, customer.Postcode);
    contact.AppendFormat("\n    {0}
{1}	", customer.Telephone, customer.Website);
    remove.AppendFormat("\n    Remove {1} from the list	", removeUrl, customer.Name);
}

        Company
        @Html.Raw(company.ToString())

        Address
        @Html.Raw(address.ToString())

        Contact Details
        @Html.Raw(contact.ToString())

        Remove
        @Html.Raw(remove.ToString())

Or does it not matter if there is only going to be a maximum of ten installers

Stay with the first solution:

It is easier to read.

Using such a template is the least-obfuscated method to generate the output HTML, as the structure of the template directly corresponds to the structure of the output.

Another important aspect is that when embedding HTML into C# strings, you have to use many escapes.
This makes it harder to read.
I would expect the first template to have comparable or better performance.

Instead of micro-optimizing, one should usually optimize for maintainability first, and let the compiler do its thing. Before optimizing for performance, do a benchmark to find real bottlenecks.

I strongly suspect that the template engine is implemented in a non-moronic way, which means that internally a string builder is used.
Your second solution on the other hand incurs additional overhead by maintaining multiple string builders which are then joined to an intermediate string which are then stuffed into the template – that intermediate string is unnecessary.

But what about having fewer loops? The answer is that it doesn't matter from a theoretical perspective: Executing

foreach (x in things)
    a(x);
foreach (x in things)
    b(x);
foreach (x in things)
    c(x);

does the same amount of work as

foreach (x in things)
{
    a(x);
    b(x);
    c(x);
}

In practice there is some small overhead per loop, but this overhead is usually negligible when compared with the actual work done inside the loop. Do a benchmark to see if this overhead matters.
It appears to be more secure.

In your second solution, you directly interpolate arbitary strings into the HTML via AppendFormat.
Unless you have very good input validation, this would allow for spectacular injection attacks.
Better be safe than sorry, and escape everything.
This is much easier if you use the templating engine rather than doing everything yourself – it's too easy to forget something.

There is another consideration from an UX standpoint:
When reading tabular data, I expect each row to contain a record, and columns to contain different fields for that record.
This makes such data easy to parse by humans and computers alike.
Swapping the axes of your table also means that your template is ever simpler:

        Company
        Address
        Contact Details
        Remove

    @foreach (Customer c in Model.SelectedInstallers)
    {

                @c.Name

                @c.Street

                @c.City

                @c.County

                @c.Postcode

                @c.Telephone

                @c.Website

                    Remove installer

    }

While displaying data row after row scales very well to a large number of records, displaying items horizontally next to each other so that each item occupies a column makes it much easier to compare the data. However, this only scales well up to three items (in my experience), and horizontal scrolling is a big no-no.

Some notes regarding the HTML: The headings of columns or rows should use the  element. While you do this in the second template, the first template only uses plain s.
To style different rows differently, it is tedious to apply classes like first-row, even, or odd.
These not only invite name collisions, but can also be done with much less effort via CSS pseudo-classes such as :nth-child:

/* tr.first-row */
tr:first-child { ... }
/* tr.even */
tr:nth-child(even) { ... }
/* tr.odd */
tr:nth-child(odd)  { ... }

The disadvantage of this solution is that it won't work on older MSIE versions.
But in this case this wouldn't be a problem as the functionality of the table is not impaired – this is an example of progressive enhancement.


Regular Expression to remove blank lines This is basically my first Ruby class, which is to remove all empty lines and blank times from a .txt file. A simple test case is also included.

I tested the code and it works fine. But I did not have much idea about if the code style is good, or if there are some potential issues. Can someone please do a quick review and kindly provide some feedback?

Is there any potential issue in the code? did I miss some boundary check?
Is the code style good in general? e.g, name convention, indent, etc.
what can I do if I want to make it more professional? e.g., better choice of functions, error-handling, object-oriented, test-driven,  etc.

require 'test/unit'

class  BlankLineRemover
  def initialize()
    puts "BlankLineRemover initialized."
  end

  def generate(input_filename, output_filename)

    reader = File.read(input_filename)

    f = File.new(output_filename, "w")

    f.puts remove(reader)

    f.close

  end

  def  remove(stringToremove)

    regEx = /^[\s]*$\n/

    stringToReturn = stringToremove.gsub(regEx, '')
    stringToReturn.strip!

    if stringToReturn.length >= 1
      return stringToReturn
    else
      return ""
    end
  end

end

class TestCleaner < Test::Unit::TestCase
  def test_basic

    sInput1 = "line1\n\t\nline4\n\tline5\n"
    sExpectedOutput1 = "line1\nline4\n\tline5"

    sInput2=""
    sExpectedOutput2 = ""

    sInput3="\n\t\t\n"
    sExpectedOutput3 = ""

    testCleaner = BlankLineRemover.new
    assert_equal(sExpectedOutput1, testCleaner.remove(sInput1))
    assert_equal(sExpectedOutput2, testCleaner.remove(sInput2))
    assert_equal(sExpectedOutput3, testCleaner.remove(sInput3))

  end
end

unless ARGV.length == 2
  puts "Usage: ruby Blanker.rb input.txt output.txt\n"
  exit
end

simpleRemover = BlankLineRemover.new

simpleRemover.generate(ARGV[0],ARGV[1])

What follows is a fairly harsh critique, but I'm hoping that that's what you're looking for as you seem to want to improve you ruby and programming style. Ruby is a lovely language, but getting to know it takes some time - eventually you'll realise that just about everything you write used to take twice as many lines. So, a few pointers:

Variables

Generally in ruby normal variables are underscored. So instead of aVariableName you should probably use a_variable_name. Both will work, but the latter would fit the Ruby style better.

Additionally you choice of variable names seems to go from overly verbose to the opposite. Some are great, but others leave my guessing.

For example where you write:

reader = File.read(input_filename)

I would prefer to write:

in_file = File.read(in_filename)

Functions

Like your variables, the naming of your functions could do with a touch-up. Your functions do not make use of the implicit return that Ruby offers, something that is often considered a better practice.

For example where you write:

  def  remove(stringToremove)

    regEx = /^[\s]*$\n/

    stringToReturn = stringToremove.gsub(regEx, '')
    stringToReturn.strip!

    if stringToReturn.length >= 1
      return stringToReturn
    else
      return ""
    end
  end

In this regard I might have written:

  def remove(stringToremove)

    regEx = /^[\s]*$\n/

    stringToReturn = stringToremove.gsub(regEx, '').strip
  end

Also on your initialize function you include brackets in the definitions this is not required, and is discouraged. Additionally you don't need to include a initializer if there's nothing for it to do.

Rubyness

Some things you might want to consider further as you do more programming in ruby are:

Chaining. For example you can do a_string.gsub("some", "substitution").strip.reverse and other such things all in one line and still keep readability high. Obviously there's a point where it's too much, but that's your judgement.
Your code formatting is a bit out. In a lot of places you seem to be using extra spaces. For example you class definition is class  BlankLineRemover and should really be class BlankLineRemover. You might be using soft-tabs here, and if that's the case - please don't.
You spacing is also very generous. Unless you're separating code into particular chunks then you can skip a few newlines.
This is a real nit-pick, but generally using .size is considered better than .length.

Rewrite

Here's your main class rewritten the way I would have gone about it.

class BlankLineRemover
  def run(in_filename, out_filename)
    File.open(out_filename, "w") do |out_file|
      out_file.write clean(File.read(in_filename))
    end
  end

  def clean(input)
    input.gsub(/^[\s]*$\n/, "").strip
  end
end


Edit one table from another table in a Windows Form DataGridView I have a Sales Order Program that allows each customer to have their own Part Number for one of our Stock Items. The Customer Part Numbers are stored in a separate table. I need to make this as low impact on the User/UI as possible.
As a working model I have come up with a related demo using Person for the Order Table and a Table called NickNames that should represent the Customer Part Number Table.

public class Person
{
    private string _sobriquet;

    public Person()
    {
        NickName = new NickName(Id);
    }

    [Key]
    public int Id { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public int NickNameId { get; set; }

    [ForeignKey("NickNameId")]
    public virtual NickName NickName { get; set; }

    public string Sobriquet
    {
        get { return _sobriquet; }
        set
        {
            _sobriquet = value;
            NickName.PersonId = Id;
            NickName.Sobriquet = _sobriquet;
        }
    }
}

public class NickName
{
    public NickName(int personId)
    {
        PersonId = personId;
    }

    protected NickName()
    {

    }

    [Key]
    public int PersonId { get; set; }

    public string Sobriquet { get; set; }
}

This does what I wanted... by Changing the Sobriquet property on the Person table the NickName Table is updated, but I am sure there is a better or cleaner way.

As I explain here, you should never initialize reference navigation properties in an entity's constructor. They prevent EF from loading the reference from the database and they impair relationship fixup.

This initialized navigation property ties your Person class to one specific use case: setting Sobriquet. For other cases where loading of the Nickname is required, it has become useless, or crippled at best.

Another downside of your implementation is that you'll never see a Sobriquet until you set it, because _sobriquet will never be set by EF. (I assume that the real property has a [NotMapped] attribute; it can't possibly be mapped).

And then of course there is this golden rule to keep unexpected behavior and side effects away from property setters (and getters, for that matter). By setting Sobriquet, one expects to set Sobriquet only. It is not expected that NickName.PersonId is set too.

Finally, in the two instances where you set NickName.PersonId, i.e. in the property setter and in new NickName(Id), the value of Person.Id can be anything. It's highly unlikely that in every imaginable scenario, Person will have its Id value on time. I'm even pretty confident that in the constructor this will never be the case.

So, as usual, if you want your view to show anything that differs from the model classes, use a view model (or DTO) and map it from/to the model classes, for example using AutoMapper.


Asks the user for an input, stores and prints it to the console c# My code is very simple, it asks for an input then stores the input. I am new to c# and wanted to know if it was possible to both ask and accept the input on one line.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication
{
    class Program
    {
        static void Main()
        {
            var input = GetInput();
            Console.WriteLine("Your input:\n{0}", input);

        }
        static string GetInput()
        {
            Console.WriteLine("Please input somthing");
            var input = Console.ReadLine();
            return input;

        }
    }
}

All feedback welcome!

You sure can, you can use something even smaller such as:

using System;

namespace ConsoleApplication
{
    class Program
    {
        static void Main()
        {
            Console.WriteLine("Please input something");
            Console.WriteLine("Your input: \n{0}", Console.ReadLine());
            Console.ReadLine(); //stops the console from closing
        }
    }
}


Odd Occurrence Array I am trying to use a dictionary in Odd Occurrence. Any suggestions on how I could optimize the program? Codility is giving me a result of 66%.

var dictionary = new Dictionary<int, short>();
int oddVal = 0; //Collect the number with Odd Occurrence

//Add the numbers in the dictionary with value 1-odd pair or 2-pair
if(A.Length != 0)
{
    for (var i = 0; i < A.Length; i++)
    {
        if (dictionary.ContainsKey(A[i]))
            dictionary[A[i]] = 2;
        else
            dictionary.Add(A[i], 1);

    }
    //review with the dictionary entry to see which key is the odd occurrence 1
    //Once odd occurrence is determined then break out of the loop
    foreach (KeyValuePair<int, short> entry in dictionary)
    {
        if (entry.Value == 1)
        {
            oddVal = entry.Key;
            break;
        }
    }

    return (int)oddVal;
}

else
    return 0;

For one I don't believe your code will cover all the possible values.  If the values passed in are 9,3,9,3,9,7,7 I believe they would want 9 to be the result since there is an odd number of 9.  Currently you are checking for a single value.

By using a dictionary you first must add all the values to it then loop over it again.  Another option would be to check if the key exist.  If not then add it but if it does then remove it.  Then all that would be left if the "odd occurrences".  If we do it that way we don't even need to have the value part of the dictionary just the key part.  If we just need the key part of the dictionary then we should use a HashSet.

so the code would look something like this (I didn't run this code just typed in here)

foreach (var item in A)
{
   if (!hashset.Add(item))
   {
       hashset.Remove(item);
   }
}

return hashset.Single();

I never used codility so I don't know what they would score this solution.

Also as a side note there is no reason to cast the oddval in your code as an int in the return statement.  It's already declared as an int.


Input any time, output timezones that are currently that time I made a program that will take in a time and a weekday, and return which parts of the
world are at times closest to the one specified. Part of the program focuses on extracting a date and country out of a user input.
Here are some example inputs and outputs:
"In what part of the world is it 4:00 PM Friday right now?" (datetime.datetime(2021, 1, 1, 16, 0), 'World')
"Where in America is it 6:40 AM Thu?"                       (datetime.datetime(2020, 12, 31, 6, 40), 'America')
"It is 10:30 pm fri in which part of Asia?"                 (datetime.datetime(2021, 1, 1, 22, 30), 'Asia')
"Where would the time be 10:10 AM Sat?"                     (datetime.datetime(2021, 1, 2, 10, 10), 'World')
"Which state of brazil has the time 8:30 AM Saturday now?"  (datetime.datetime(2021, 1, 2, 8, 30), 'Brazil')

The reason that the only part of the date that needs to be given is the weekday,
is because the maximum possible days in difference between a timezone and the timezone
the user is currently at is one day, either ahead or behind.
So the program checks if the weekday given is at the current day, one day before the current day, or one day after the current day, and can return the rest of the date.
Here is my code:
import pytz
from datetime import datetime, timedelta
import re

def return_words(string):
    return re.findall(r'\b\S+\b', string.lower())

def string_to_datetime(string):
    weekdays = ["sunday", "monday", "tuesday", "wednesday", "thursday", "friday", "saturday"]
    string = string.lower()
    words = sorted(return_words(string), key=len)[::-1]
    countries = [t.split("/")[0] for t in pytz.all_timezones]

    for word in words:
        in_day = [weekday for weekday in weekdays if word in weekday]
        if len(in_day) == 1: # Make sure that the word found is a weekday name, not just something like "day"
            weekday = in_day[0]
            break
    else:
        return

    for word in words:
        if word.title() in countries:
            country = word.title()
            break
    else:
        country = "World" # If no country name found, use "World"

    date = datetime.now()
    one_day = timedelta(days=1)
    if (datetime.now() + one_day).strftime("%A").lower() == weekday:
        date += one_day
    elif (datetime.now() - one_day).strftime("%A").lower() == weekday:
        date -= one_day
    elif datetime.now().strftime("%A").lower() != weekday:
        return

    year, month, day = date.year, date.month, date.day
    hours_minutes = re.findall("\d+\:\d+", string)[0]

    half = re.findall(f"(?<={hours_minutes}).{{0,3}}[ap]m", string) # Play it safe, and only extract the am or pm that is at most 3 steps away from the hours:minutes
    if half: # If found an am or pm
        half = half[0].strip()
    else: # If not found when playing it safe, resort to finding any am or pm in the string
        half = re.findall("[ap]m", string)[0].strip()
    string = f"{year} {month} {day} {hours_minutes} {half}"
    try:
        return datetime.strptime(string, "%Y %m %d %I:%M %p"), country
    except ValueError:
        return

while True:
    print(string_to_datetime(input("Input your string >>> ")))

Can you show me how to improve the efficiency of my code? Also, any tips on tidying up would be of big help.

in_day = [weekday for weekday in weekdays if word in weekday]
    if len(in_day) == 1:

seems odd. You make the entire list, put it in memory, take its length and then throw it away. Consider instead
n_matching = sum(1 for weekday in weekdays if word in weekday)
if n_matching == 1:

This loop:
for word in words:
    in_day = [weekday for weekday in weekdays if word in weekday]
    if len(in_day) == 1: # Make sure that the word found is a weekday name, not just something like "day"
        weekday = in_day[0]
        break

can be simplified if you use set operations; lower everything in words before this:
countries = {t.split("/", 1)[0].lower() for t in pytz.all_timezones}
try:
    country = next(iter(countries & set(words))).title()
except StopIteration:
    country = "World" # If no country name found, use "World"


And the little one said "Roll over" Below is some code I have written today to test out some bits I have learnt. It isn't much nor is it spectacular. Please critique and let me know what I could/should have done differently or anything I could do to improve it so far.

package easy8;

import java.util.Scanner;

public class song99bottles {

public static void main(String[] args) {

// Declare a reference variable of type song99bottles - new object
song99bottles go = new song99bottles();
// calls the method "queston" for object "go".
go.question();

}

public void question(){

    song99bottles start = new song99bottles();

    String answer;

    System.out.println("Would you like to hear a nursery rhyme?\nPlease enter yes or no:");
    Scanner input = new Scanner(System.in);
    answer = input.next();
    if(answer.equalsIgnoreCase("yes")){
        start.lyrics();
    } else{
        System.out.println("Bye");
    }

}

public void lyrics() {

    int peeps  = 10;
    String intro = "There were ";
    String intro2 = " in the bed and the little one said rollover";
    String fall = "\nSo they all rolled over and 1 fell out.";
    String end = "And the little said 'I'm lonely'.";

    while (peeps > 0){
        System.out.println( intro + peeps + intro2 + fall);
        peeps--;
        if (peeps == 1){
            System.out.println(end);
            return;
                        }
    }

}

}

Class names should be PascalCase (song99bottles -> Song99Bottles)
You should indent your code or you will end up with a long code and you don't understand the various { } pairs (and an horrible code!)

public class song99bottles {
public static void main(String[] args) {
// Declare a reference variable of type song99bottles - new object
song99bottles go = new song99bottles();
// calls the method "queston" for object "go".
go.question();
}
// [...]

if you want to follow Code conventions, you code should look like this

public class Song99Bottles {

    public static void main(String[] args) {
        // Declare a reference variable of type song99bottles - new object
        Song99Bottles go = new Song99Bottles();
        // calls the method "queston" for object "go".
        go.question();
    }

    // [...]

Personally i follow the new-line style, but you will see with time you will get "your style".

i hope you understand the concept. Read wikipedia page.
You should give to your variables a more descriptive name, you should understand the scope of the variable by just reading the name. (song99bottles go = new song99bottles(); go? What is? a countdown? I could call it song (example))

About your code:

You create two song99bottles objects, why? One inside main and one inside question(). You should move question code inside main because song99bottles class should just play the song, the main will take care if the user want or not to play the song. Then in your main, just call go.lyrics();
About lyrics() code, see Roger answer.


Finding the Bayesian estimates for the top 100 products on NutraPlanet This code fetches reviews for the top 100 products on NutraPlanet, finds the Bayesian estimate for each average review, then prints out all reviews sorted by that Bayesian estimate.

How can I improve this code (correctness, elegance, best practices, etc)?

<?php

// echo "Remove exit from script to run."; exit;

define("MINIMUM_NUMBER_OF_VOTES_TO_SAVE_PRODUCT", 2);

$locations = array("http://www.nutraplanet.com/top_100");

$reviewUrlFragments = array_unique(getReviewUrlFragments($locations));

$products = getProducts(
        $reviewUrlFragments,
        MINIMUM_NUMBER_OF_VOTES_TO_SAVE_PRODUCT);

calculateBayesianEstimate($products);

usort($products, function ($a, $b) {
    if ($a["bayesian_estimate"] == $b["bayesian_estimate"]) {
        return 0;
    }
    return ($a["bayesian_estimate"] < $b["bayesian_estimate"]) ? 1 : -1;
});

printReviews($products);

function getReviewUrlFragments($locations) {
    $reviewUrlFragments = array();

    foreach($locations as $location) {
        $page = @file_get_contents($location);

        preg_match_all("%/product/(.*?)\.html%is", $page, $matches);

        $reviewUrlFragments = array_merge($reviewUrlFragments, $matches[1]);
    }

    return $reviewUrlFragments;
}

function getProducts($productReviewUrlFragments, $minimumNumberOfVotesToSaveProduct) {
    $products = array();

    foreach($productReviewUrlFragments as $productReviewUrlFragment) {
        $reviewUrl = "http://www.nutraplanet.com/manufacturer/" . $productReviewUrlFragment . "/reviews";

        $page = @file_get_contents($reviewUrl);

        preg_match_all("%&nbsp;(.*?)	%is", $page, $matches);

        $product = array();
        $product['name'] = "http://www.nutraplanet.com/product/".$productReviewUrlFragment.".html";

        $votes = getVotes($matches[1]);

        $product["vote_distribution"] = $votes["votes"];
        $product["review_average"] = $votes["average"];
        $product["num_votes"] = $votes["num_votes"];

        // Only add if there is a vote for the product
        if ($product["num_votes"] >= $minimumNumberOfVotesToSaveProduct) {
            $products[] = $product;
        }
    }

    return $products;
}

function getVotes($match) {
    $votes = array();
    $sum_votes = 0;
    $total_votes = 0;
    $vote_star = 1;

    // In ascending order, 1-star to 5
    foreach($match as $vote_count) {
        $votes[] = $vote_count;
        $total_votes += $vote_count;
        $sum_votes += ($vote_count * $vote_star++);
    }

    $product["votes"] = $votes;
    $product["average"] = ($total_votes < 1) ? 0 : ($sum_votes / $total_votes);
    $product["num_votes"] = $total_votes;

    return $product;
}

function calculateBayesianEstimate(&$products) {
    // See bottom of page: http://www.imdb.com/chart/top
    $m = getMinNumberOfVotes($products);
    $C = getAverageReviewAverage($products);

    for ($i = 0, $len = count($products); $i < $len; $i++) {
        $R = $products[$i]["review_average"];
        $v = $products[$i]["num_votes"];
        $products[$i]["bayesian_estimate"] = (($v / ($v+$m)) * $R) + (($m / ($v+$m)) * $C);
    }
}

function getAverageReviewAverage($products) {
    $total = 0;

    foreach($products as $product) {
        $total += $product["review_average"];
    }

    return $total / count($products);
}

function getMinNumberOfVotes($products) {
    foreach($products as $product) {
        if (!isset($minVote) || ($product["num_votes"] < $minVote)) {
            $minVote = $product["num_votes"];
        }
    }

    if (!isset($minVote)) {
        $minVote = 0;
    }

    return $minVote;
}

function printReviews($products) {
    echo "
";
    foreach($products as $product) {
        $product["vote_distribution"] = implode("	", $product["vote_distribution"]);
        $product = implode("	", $product);
        echo "
" . $product . "
";
    }
    echo "
";
}
?>

I just put this together, so I haven't tested it or anything, there might be bugs and someone else PLEASE come along and improve upon it, but I think this might be a good starting point to answering this question:

OOP version of OP's code:

$ReviewPrinter = new ReviewPrinter("http://www.nutraplanet.com/top_100", 2);
echo $ReviewPrinter->printReviews();

class ReviewPrinter{
    public $products;
    public $locations;
    public $reviewUrlFragments;
    public $minimumNumberOfVotesToSaveProduct;

    public function __construct($locations, $minimumNumberOfVotesToSaveProduct){
        if(empty($locations) || empty($minimumNumberOfVotesToSaveProduct)){
            return false;
        } else {
            $this->minimumNumberOfVotesToSaveProduct = $minimumNumberOfVotesToSaveProduct;
            $this->locations = array($locations);
            $this->setReviewUrlFragments();
            $this->products = $this->getProducts();
            $this->calculateBayesianEstimate();
            $this->sortProducts();
        }
    }

    public function sortProducts(){
        usort($this->products, function ($a, $b) {
            if ($a["bayesian_estimate"] == $b["bayesian_estimate"]) {
                return 0;
            }
            return (($a["bayesian_estimate"] < $b["bayesian_estimate"]) ? 1 : -1);
        });
    }

    public function setReviewUrlFragments(){
        $localReviewUrlFragments = array();

        foreach($this->locations as $location) {
            preg_match_all("%/product/(.*?)\.html%is", @file_get_contents($location), $matches);
            $localReviewUrlFragments = array_merge($localReviewUrlFragments, $matches[1]);
        }

        $this->reviewUrlFragments = array_unique($localReviewUrlFragments);
    }

    public function getProducts(){
        $functionScopeProducts = array();

        foreach($this->reviewUrlFragments as $productReviewUrlFragment) {
            $reviewUrl = "http://www.nutraplanet.com/manufacturer/" . $productReviewUrlFragment . "/reviews";

            preg_match_all("%&nbsp;(.*?)	%is", @file_get_contents($reviewUrl), $matches);

            $localProducts = array();
            $localProducts['name'] = "http://www.nutraplanet.com/product/".$productReviewUrlFragment.".html";

            $votes = $this->getVotes($matches[1]);

            $localProducts["vote_distribution"] = $votes["votes"];
            $localProducts["review_average"] = $votes["average"];
            $localProducts["num_votes"] = $votes["num_votes"];

            // Only add if there is a vote for the product
            if ($localProducts["num_votes"] >= $this->minimumNumberOfVotesToSaveProduct) {
                $functionScopeProducts[] = $localProducts;
            }
        }

        return $functionScopeProducts;
    }

    public function getVotes($match){
        $votes = array();
        $sum_votes = 0;
        $total_votes = 0;
        $vote_star = 1;

        // In ascending order, 1-star to 5
        foreach($match as $vote_count) {
            $votes[] = $vote_count;
            $total_votes += $vote_count;
            $sum_votes += ($vote_count * $vote_star++);
        }

        $product["votes"] = $votes;
        $product["average"] = ($total_votes < 1) ? 0 : ($sum_votes / $total_votes);
        $product["num_votes"] = $total_votes;

        return $product;
    }

    public function calculateBayesianEstimate(){
        /**
         * Algorithm used:  (WR) = (v ÷ (v+m)) × R + (m ÷ (v+m)) × C
         * where:
         * WR = Weighted rating
         * R  = Average rating
         * v  = Number of votes
         * m  = Minimum votes required
         * C  = Mean vote across the whole report
         */
        $m = $this->getMinNumberOfVotes();
        $C = $this->getAverageReviewAverage();

        for ($i = 0, $len = count(&$this->products); $i < $len; $i++) {
            $R = &$this->products[$i]["review_average"];
            $v = &$this->products[$i]["num_votes"];
            &$this->products[$i]["bayesian_estimate"] = (($v / ($v+$m)) * $R) + (($m / ($v+$m)) * $C);
        }
    }

    public function getAverageReviewAverage(){
        $total = 0;

        foreach($this->products as $product) {
            $total += $product["review_average"];
        }

        return $total / count($this->products);
    }

    public function getMinNumberOfVotes(){
        # NOTE: Where is $minVote set or created?
        foreach($this->products as $product) {
            if (!isset($minVote) || ($product["num_votes"] < $minVote)) {
                $minVote = $product["num_votes"];
            }
        }

        if (!isset($minVote)) {
            $minVote = 0;
        }

        return $minVote;
    }

    public function printReviews(){
        $output = "
";
        foreach($this->products as $product) {
            $product["vote_distribution"] = implode("	", $product["vote_distribution"]);
            $product = implode("	", $product);
            $output .= "
" . $product . "
";
        }
        $output .= "
";

        return $output;
    }
}

Obviously, there should be separation of concerns, which this does not implement, etc.

++++
EDIT
++++

Implemented comments

// Usage:
$ReviewPrinter = new ReviewPrinter("http://www.nutraplanet.com/top_100", 2);
echo $ReviewPrinter->printReviews();

class ReviewPrinter{
    private $products;
    private $locations;
    private $reviewUrlFragments;
    private $minimumNumberOfVotesToSaveProduct;

    public function __construct($locations, $minimumNumberOfVotesToSaveProduct){
        if(empty($locations) || empty($minimumNumberOfVotesToSaveProduct)){
            $c = 2 - (empty($locations) ? 1 : 0) - (empty($minimumNumberOfVotesToSaveProduct) ? 1 : 0);
            throw new Exception("Construct expects 2 parameters, $c given.");
        } else {
            $this->minimumNumberOfVotesToSaveProduct = $minimumNumberOfVotesToSaveProduct;
            $this->locations = array($locations);
            $this->setReviewUrlFragments();
            $this->products = $this->getProducts();
            $this->calculateBayesianEstimate();
            $this->sortProducts();
        }
    }

    private function sortProducts(){
        usort($this->products, function ($a, $b) {
            if ($a["bayesian_estimate"] == $b["bayesian_estimate"]) {
                return 0;
            }
            return (($a["bayesian_estimate"] < $b["bayesian_estimate"]) ? 1 : -1);
        });
    }

    private function setReviewUrlFragments(){
        $localReviewUrlFragments = array();

        foreach($this->locations as $location) {
            preg_match_all("%/product/(.*?)\.html%is", @file_get_contents($location), $matches);
            $localReviewUrlFragments = array_merge($localReviewUrlFragments, $matches[1]);
        }

        $this->reviewUrlFragments = array_unique($localReviewUrlFragments);
    }

    private function getProducts(){
        $functionScopeProducts = array();

        foreach($this->reviewUrlFragments as $productReviewUrlFragment) {
            $reviewUrl = "http://www.nutraplanet.com/manufacturer/" . $productReviewUrlFragment . "/reviews";

            preg_match_all("%&nbsp;(.*?)	%is", @file_get_contents($reviewUrl), $matches);

            $localProducts = array();
            $localProducts['name'] = "http://www.nutraplanet.com/product/".$productReviewUrlFragment.".html";

            $votes = $this->getVotes($matches[1]);

            $localProducts["vote_distribution"] = $votes["votes"];
            $localProducts["review_average"] = $votes["average"];
            $localProducts["num_votes"] = $votes["num_votes"];

            // Only add if there is a vote for the product
            if ($localProducts["num_votes"] >= $this->minimumNumberOfVotesToSaveProduct) {
                $functionScopeProducts[] = $localProducts;
            }
        }

        return $functionScopeProducts;
    }

    private function getVotes($match){
        $votes = array();
        $sum_votes = 0;
        $total_votes = 0;
        $vote_star = 1;

        // In ascending order, 1-star to 5
        foreach($match as $vote_count) {
            $votes[] = $vote_count;
            $total_votes += $vote_count;
            $sum_votes += ($vote_count * $vote_star++);
        }

        $product["votes"] = $votes;
        $product["average"] = ($total_votes < 1) ? 0 : ($sum_votes / $total_votes);
        $product["num_votes"] = $total_votes;

        return $product;
    }

    private function calculateBayesianEstimate(){
        /**
         * Algorithm used:  (WR) = (v ÷ (v+m)) × R + (m ÷ (v+m)) × C
         * where:
         * WR = Weighted rating
         * R  = Average rating
         * v  = Number of votes
         * m  = Minimum votes required
         * C  = Mean vote across the whole report
         */
        $m = $this->getMinNumberOfVotes();
        $C = $this->getAverageReviewAverage();

        for ($i = 0, $len = count(&$this->products); $i < $len; $i++) {
            $R = &$this->products[$i]["review_average"];
            $v = &$this->products[$i]["num_votes"];
            &$this->products[$i]["bayesian_estimate"] = (($v / ($v+$m)) * $R) + (($m / ($v+$m)) * $C);
        }
    }

    private function getAverageReviewAverage(){
        $total = 0;

        foreach($this->products as $product) {
            $total += $product["review_average"];
        }

        return $total / count($this->products);
    }

    private function getMinNumberOfVotes(){
        # NOTE: Where is $minVote set or created?
        foreach($this->products as $product) {
            if (!isset($minVote) || ($product["num_votes"] < $minVote)) {
                $minVote = $product["num_votes"];
            }
        }

        if (!isset($minVote)) {
            $minVote = 0;
        }

        return $minVote;
    }

    public function printReviews(){
        $output = "
";
        foreach($this->products as $product) {
            $product["vote_distribution"] = implode("	", $product["vote_distribution"]);
            $product = implode("	", $product);
            $output .= "
" . $product . "
";
        }
        $output .= "
";

        return $output;
    }
}


Dispose pattern - DisposableObject I am trying to make an universal implementation of IDisposable (as a base class):

public abstract class DisposableObject : IDisposable
{
    private bool hasUnmanagedResources;
    private bool baseDisposeManagedResourcesCalled;
    private bool baseDisposeUnmanagedResourcesCalled;

    protected bool IsDisposed { get; private set; }
    protected bool IsDisposing { get; private set; }

    public DisposableObject()
        : this(false)
    {
    }

    public DisposableObject(bool hasUnmanagedResources)
    {
        this.hasUnmanagedResources = hasUnmanagedResources;

        if (!hasUnmanagedResources)
        {
            GC.SuppressFinalize(this);
        }
    }

    ~DisposableObject()
    {
        Dispose(false);
    }

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

    protected virtual void DisposeManagedResources()
    {
        if (!IsDisposing)
        {
            throw new InvalidOperationException(
                "In order to dispose an object call the Dispose method. Do not call DisposeManagedResources method directly." +
                    GetTypeString());
        }

        baseDisposeManagedResourcesCalled = true;
    }

    protected virtual void DisposeUnmanagedResources()
    {
        if (!IsDisposing)
        {
            throw new InvalidOperationException(
                "In order to dispose an object call the Dispose method. Do not call DisposeUnmanagedResources method directly." +
                    GetTypeString());
        }

        baseDisposeUnmanagedResourcesCalled = true;
    }

    protected void HasUnmanagedResources()
    {
        if (!hasUnmanagedResources)
        {
            hasUnmanagedResources = true;
            GC.ReRegisterForFinalize(this);
        }
    }

    protected void ThrowIfDisposed()
    {
        if (IsDisposed)
        {
            throw new ObjectDisposedException("Object is already disposed." + GetTypeString());
        }

        if (IsDisposing)
        {
            throw new ObjectDisposedException("Object is being disposed." + GetTypeString());
        }
    }

    private void Dispose(bool disposeManagedResources)
    {
        if (IsDisposed)
        {
            throw new InvalidOperationException("Dispose called on an object that is already disposed." + GetTypeString());
        }

        if (IsDisposing)
        {
            throw new InvalidOperationException("Dispose called on an object that is currently disposing." + GetTypeString());
        }

        IsDisposing = true;

        if (disposeManagedResources)
        {
            DisposeManagedResources();
            VerifyBaseDisposeManagedResourcesCalled();
        }

        if (hasUnmanagedResources)
        {
            DisposeUnmanagedResources();
            VerifyBaseDisposeUnmanagedResourcesCalled();
        }

        IsDisposed = true;
        IsDisposing = false;
    }

    private void VerifyBaseDisposeManagedResourcesCalled()
    {
        if (!baseDisposeManagedResourcesCalled)
        {
            throw new InvalidOperationException("DisposeManagedResources of the base class was not called." + GetTypeString());
        }
    }

    private void VerifyBaseDisposeUnmanagedResourcesCalled()
    {
        if (!baseDisposeUnmanagedResourcesCalled)
        {
            throw new InvalidOperationException("DisposeUnmanagedResources of the base class was not called." + GetTypeString());
        }
    }

    private string GetTypeString()
    {
        string typeName = GetType().FullName;
        return " Type: " + typeName;
    }
}

By default, the object is removed from the finalization queue (in the DisposableObject constructor). If some inheriting class has an unmanaged resource then it should invoke HasUnmanagedResources method to add the object back to the finalization queue.

The most important question is: can I invoke GC.SuppressFinalize(this) in the constructor?.

How do you find this implementation?

Firstly, to address inheritance concerns, I use a t4 template to get around the single inheritance problem. ie. for every class where I need to introduce dispose support, I generate the an abstract base class that inherits from that class. Works a treat.

Now, to your code. Here are the things that jump out at me:

Constructors should be protected because the base class is abstract. This would make the code clearer to me, but YMMV.
Lose the whole unmanaged resource tracking. It adds complexity without value. Just call SuppressFinalize() regardless - it's the recommended pattern, anyway.
Your code is not thread-safe. Consider, for example, what happens if multiple threads call Dispose() simultaneously. You're got the potential for multiple calls to DisposeManagedResources() etcetera.

FWIW, here is an example Disposable base class generated by my t4 template:

// Generated by t4 template in Utility project. DO NOT EDIT!
#pragma warning disable 1591, 0612
#region Designer generated code

namespace Foo
{
    using System;
    using System.Diagnostics;
    using System.Diagnostics.CodeAnalysis;
    using System.Globalization;
    using System.Threading;
    using Kent.Boogaart.HelperTrinity.Extensions;

    /// <summary>
    /// A useful base class for disposable objects that inherit from <see cref="object"/>.
    /// </summary>
    /// <remarks>
    /// <para>
    /// This base class simplifies the implementation of disposable objects by providing common functionality such as thread-safety on <see cref="Dispose"/> calls,
    /// and a <see cref="Disposing"/> event for pre-disposal notification.
    /// </para>
    /// </remarks>
    public abstract class DisposableBase : object, IDisposable
    {
        private const int DisposalNotStarted = 0;
        private const int DisposalStarted = 1;
        private const int DisposalComplete = 2;

#if DEBUG
        // very useful diagnostics when a failure to dispose is detected
        private readonly StackTrace creationStackTrace;
#endif

        // see the constants defined above for valid values
        private int disposeStage;

#if DEBUG
        /// <summary>
        /// Initializes a new instance of the DisposableBase class.
        /// </summary>
        protected DisposableBase()
        {
            this.creationStackTrace = new StackTrace(1, true);
        }

        /// <summary>
        /// Finalizes an instance of the DisposableBase class.
        /// </summary>
        [SuppressMessage("Microsoft.Design", "CA1063", Justification = "The enforced behavior of CA1063 is not thread-safe or full-featured enough for our purposes here.")]
        ~DisposableBase()
        {
            var message = string.Format(CultureInfo.InvariantCulture, "Failed to proactively dispose of object, so it is being finalized: {0}.{1}Object creation stack trace:{1}{2}", this.ObjectName, Environment.NewLine, this.creationStackTrace);
            Debug.Assert(false, message);
            this.Dispose(false);
        }
#endif

        /// <summary>
        /// Occurs when this object is about to be disposed.
        /// </summary>
        public event EventHandler Disposing;

        /// <summary>
        /// Gets a value indicating whether this object is in the process of disposing.
        /// </summary>
        protected bool IsDisposing
        {
            get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalStarted, DisposalStarted) == DisposalStarted; }
        }

        /// <summary>
        /// Gets a value indicating whether this object has been disposed.
        /// </summary>
        protected bool IsDisposed
        {
            get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalComplete, DisposalComplete) == DisposalComplete; }
        }

        /// <summary>
        /// Gets a value indicating whether this object has been disposed or is in the process of being disposed.
        /// </summary>
        protected bool IsDisposedOrDisposing
        {
            get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalNotStarted, DisposalNotStarted) != DisposalNotStarted; }
        }

        /// <summary>
        /// Gets the object name, for use in any <see cref="ObjectDisposedException"/> thrown by this object.
        /// </summary>
        /// <remarks>
        /// Subclasses can override this property if they would like more control over the object name appearing in any <see cref="ObjectDisposedException"/>
        /// thrown by this <c>DisposableBase</c>. This can be particularly useful in debugging and diagnostic scenarios.
        /// </remarks>
        /// <returns>
        /// The object name, which defaults to the class name.
        /// </returns>
        protected virtual string ObjectName
        {
            get { return this.GetType().FullName; }
        }

        /// <summary>
        /// Disposes of this object, if it hasn't already been disposed.
        /// </summary>
        [SuppressMessage("Microsoft.Design", "CA1063", Justification = "The enforced behavior of CA1063 is not thread-safe or full-featured enough for our purposes here.")]
        [SuppressMessage("Microsoft.Usage", "CA1816", Justification = "GC.SuppressFinalize is called indirectly.")]
        public void Dispose()
        {
            if (Interlocked.CompareExchange(ref this.disposeStage, DisposalStarted, DisposalNotStarted) != DisposalNotStarted)
            {
                return;
            }

            this.OnDisposing();
            this.Disposing = null;

            this.Dispose(true);
            this.MarkAsDisposed();
        }

        /// <summary>
        /// Verifies that this object is not in the process of disposing, throwing an exception if it is.
        /// </summary>
        protected void VerifyNotDisposing()
        {
            if (this.IsDisposing)
            {
                throw new ObjectDisposedException(this.ObjectName);
            }
        }

        /// <summary>
        /// Verifies that this object has not been disposed, throwing an exception if it is.
        /// </summary>
        protected void VerifyNotDisposed()
        {
            if (this.IsDisposed)
            {
                throw new ObjectDisposedException(this.ObjectName);
            }
        }

        /// <summary>
        /// Verifies that this object is not being disposed or has been disposed, throwing an exception if either of these are true.
        /// </summary>
        protected void VerifyNotDisposedOrDisposing()
        {
            if (this.IsDisposedOrDisposing)
            {
                throw new ObjectDisposedException(this.ObjectName);
            }
        }

        /// <summary>
        /// Allows subclasses to provide dispose logic.
        /// </summary>
        /// <param name="disposing">
        /// Whether the method is being called in response to disposal, or finalization.
        /// </param>
        protected virtual void Dispose(bool disposing)
        {
        }

        /// <summary>
        /// Raises the <see cref="Disposing"/> event.
        /// </summary>
        protected virtual void OnDisposing()
        {
            this.Disposing.Raise(this);
        }

        /// <summary>
        /// Marks this object as disposed without running any other dispose logic.
        /// </summary>
        /// <remarks>
        /// Use this method with caution. It is helpful when you have an object that can be disposed in multiple fashions, such as through a <c>CloseAsync</c> method.
        /// </remarks>
        [SuppressMessage("Microsoft.Usage", "CA1816", Justification = "This is a helper method for IDisposable.Dispose.")]
        protected void MarkAsDisposed()
        {
            GC.SuppressFinalize(this);
            Interlocked.Exchange(ref this.disposeStage, DisposalComplete);
        }
    }
}

#endregion Designer generated code
#pragma warning restore 1591, 0612


C++ Bit shift whole array (left) For an embedded project I want to shift a (byte) array left by a certain amount of bits. I've built this template function to do so:

template <typename T>
constexpr void shift_array_left(T *arr, const size_t size, const size_t bits, const bool zero = false) {
    const size_t chunks = bits / (8 * sizeof(T));

    if (chunks >= size) {
        if (zero) {
            memset(arr, 0, size);
        }
        return;
    }

    if (chunks) {
        memmove(arr, arr + chunks, size - chunks);
        if (zero) {
            memset(arr + size - chunks, 0, chunks);
        }
    }

    const size_t left = bits % (8 * sizeof(T));

    // If we have non directly addressable bits left we need to move the whole thing one by one.
    if (left) {
        const size_t right = (8 * sizeof(T)) - left;
        const size_t l = size - chunks - 1;
        for (size_t i = 0; i < l; i++) {
            arr[i] = ((arr[i] << left) & ~left) | (arr[i+1] >> right);
        }
        arr[l] = (arr[l] << left) & ~left;
    }
}

Some questions I have:

I inferred from the memmove and memcpy relationships that memmove doesn't guarantee zeroing out any 'gap' created. Can I do away with the zeroing parts?
This function only makes sense for unsigned integer types, what is the best way to force the compiler to only accept these, without repeating function bodies?

I'd, of course, appreciate any further feedback / criticism.

Update: Live example in compiler explorer: https://godbolt.org/g/I3qDsL

For the second question you can use type_traits(is_unsigned) and enable_if.
http://en.cppreference.com/w/cpp/types/is_unsigned
http://en.cppreference.com/w/cpp/types/enable_if

For the first question:
The main difference between memmove and memcpy is that memmove can be used to relocate a piece of memory to somewhere that overlaps the memory block that you're trying to move. And therefore the original pointer will no longer be valid. So I think it's up to you how you want your function to behave, if you want the parts to be zeroed or not or leave it too the user.


Factorial number in Rust for learning purposes I'm currently trying to learn the basics of the Rust programming language, to do this my first piece of code after the classic 'Hello World' was a simple recursive Factorial function.

This is the code:

use std::io;

fn factorial(num: i64) -> i64 {
    return if num == 1 {
        1
    }else{
        num * factorial(num - 1)
    }
}

fn trim_newline(s: &mut String){
    if s.ends_with('\n'){
        s.pop();
        if s.ends_with('\r'){
            s.pop();
        }
    }
}

fn main() {
    let mut user_input = String::new();
    println!("Welcome to rust-factorial!");
    loop {
        user_input.clear();
        println!("Please enter a number:");
        match io::stdin().read_line(&mut user_input) {
            Ok(_) => (),
            Err(_e) => {
                println!("error: {}", _e);
                continue;
            },
        };
        let num: i64 = match user_input.trim().parse::<i64>() {
            Ok(n) => n,
            Err(_e) => {
                trim_newline(&mut user_input); // remove trailing newline
                println!("'{}' is not a valid number, full error: {}", user_input, _e);
                continue;
            },
        };
        println!("{}! is equal to {}", num, factorial(num));
    }
}

I am aware of one major issue with my current approach which is that the biggest number this program can calculate is the 20th number of the sequence. Asking for a number bigger than 20 results in the following error on my machine:

Please enter a number:
24
thread 'main' panicked at 'attempt to multiply with overflow', src\main.rs:7:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: process didn't exit successfully: `target\debug\rust-factorial.exe` (exit code: 101)

I'd like to know a few things:

How can I change this code to be more "rust-like"

(by that I mean 'pythonic' but for rust)

What would the rust approach be for solving the overflow that happens with numbers past 20?

(I could either hard-code the limit or something to that effect but I'm curious whether or not there is a guideline about this from a Rust perspective)

Any and all comments on code quality, readability and potentially efficiency

Bugs and types

You should think some about what types you want to have as input and output for factorial. Negative numbers won't ever work as input, and the output won't be negative, so you should consider using an unsigned integer, like u64. Even doing this, though, there's one input that leads to an infinite loop. See if you can find it and fix the function!

Cleaning up the error handling

match io::stdin().read_line(&mut user_input) {
    Ok(_) => (),
    Err(_e) => {
        println!("error: {}", _e);
        continue;
    }
};

A variable with a leading underscore tells readers (and linters) that the variable isn't going to be used. Since we use it (with the println!), we should just call it e.

This error message should probably be in stderr, rather than stdout. This can be done just by changing it to eprintln! instead.

When only one branch of a match actually does something, if let is both easier to write and understand. So in all, I'd change this part to

if let Err(e) = io::stdin().read_line(&mut user_input) {
    eprintln!("error: {}", e);
    continue;
}

let num: i64 = match user_input.trim().parse::<i64>() {
    Ok(n) => n,
    Err(_e) => {
        trim_newline(&mut user_input); // remove trailing newline
        println!("'{}' is not a valid number, full error: {}", user_input, _e);
        continue;
    },
};

Same thing with _e vs e. You're using the variable, so it shouldn't start with an underscore. Also the same thing with eprintln!. Errors should be printed to stderr.

Since both branches of the match are used, if let is less useful. However, you might check out the answers to this question for some ideas for making this look better. For a single use like this, you're probably fine, but if you do this "unwrap or continue" behavior a lot, it can help to encapsulate it.

The i64 annotations are unnecessary. The compiler has enough information to conclude that the type of num is i64 since it's fed into factorial. If you want to keep an annotation for explicitness, that's fine, but only one of num: i64 and parse::<i64> is needed for that.

trim_newline seems to be unnecessary. You're parsing user_input.trim() anyway, so just show that to the user if there's a parsing error.

let num = match user_input.trim().parse() {
    Ok(n) => n,
    Err(e) => {
        eprintln!("'{}' is not a valid number, full error: {}", user_input.trim(), e);
        continue;
    }
};

Using standard tools for formatting and linting

One thing that goes a long way with making your code more readable and idiomatic is running cargo fmt. You may need to install it with rustup component add rustfmt.

Clippy is another standard tool for catching errors and making your code more idiomatic. It can similarly be installed with rustup using rustup component add clippy. Running it (with cargo clippy) on your program, we get the warning

warning: unneeded `return` statement
 --> src/main.rs:4:5
  |
4 | /     return if num == 1 {
5 | |         1
6 | |     }else{
7 | |         num * factorial(num - 1)
8 | |     }
  | |_____^
  |
  = note: `#[warn(clippy::needless_return)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
help: remove `return`
  |
4 |     if num == 1 {
5 |         1
6 |     }else{
7 |         num * factorial(num - 1)
8 |     }
  |

Since the return statement is the last thing in the function, it's not needed since the last expression in the block is always implicitly returned. You can simply remove the return.

Some advice on overflows

The factorial function grows superexponentially, so any fixed-width integer type is going to quickly overflow. Even switching to u128 only gets you up to an input of 34. The way I see it, you have two choices. Either

Embrace this. Return an Option (or better yet, a Result) to indicate that an overflow occurred. Using something like u64::checked_mul would be helpful for that.
Avoid overflow completely (barring memory limits) by using some sort of BigInt. The crate num_bigint has an implementation with everything you'd need here, including parsing, printing, multiplying and subtracting. You should be able to replace i64 with BigUint (or BigInt) with only a few conversions from integer literals.