Random temp folder implementation for unit testing I faced a series of problems when unit testing a tool of mine that generates files. They were caused by me putting the files in the same temporary folder and the tests running in parrallel.

Here is an exemple of the unit tests (for @shanif)

public class OutputWriterTests
{
    [Fact]
    public async Task WriteToDirectoryAsync()
    {
        using var tempFolder = new TempFolder();
        var fileWriter = new OutputWriter(tempFolder.Folder);
        var fileCount = tempFolder.Folder.GetFiles().Length;

        await fileWriter.WriteAsync("toto").ConfigureAwait(false);

        Assert.Equal(fileCount + 1, tempFolder.Folder.GetFiles().Length);
    }

    [Fact]
    public async Task AppendToFileAsync()
    {
        using var tempFolder = new TempFolder();
        var testFile = new FileInfo(Path.Combine(tempFolder.Folder.FullName, "test"));
        var fileWriter = new OutputWriter(testFile);

        await fileWriter.WriteAsync("toto").ConfigureAwait(false);

        var text = File.ReadAllText(testFile.FullName);
        Assert.Equal("toto\r\n", text);
    }
}

As a result I created this simplistic class that handles the creation and deletion of a temporary folder. As it implements IDisposable it's meant to be used inside a using block, or manually disposed.

public class TempFolder : IDisposable
{
    public DirectoryInfo Folder;

    public TempFolder(string prefix = "TempFolder")
    {
        var folderName = prefix + new Random().Next(1000000000);
        Folder = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), folderName));
    }

    public void Dispose()
    {
        Directory.Delete(Folder.FullName, true);
    }
}

You could easily wind up with collisions on the folder name if the method is called multiple times in very quick succession because of the new Random() being declared for each usage. It should be instantiated once, and since you mentioned it could be called on multiple threads, it should be mutexed onto one at a time. Finally, it's not a good idea to have fields be public - making it a property is luckily pretty easy. So:

public class TempFolder : IDisposable
{
    private static readonly Random _Random = new Random();

    public DirectoryInfo Folder { get; }

    public TempFolder(string prefix = "TempFolder")
    {
        string folderName;

        lock (_Random)
        {
            folderName = prefix + _Random.Next(1000000000);
        }

        Folder = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), folderName));
    }

    public void Dispose()
    {
        Directory.Delete(Folder.FullName, true);
    }
}

Now, what I don't address here is the issue of constructors or the Dispose() method possibly throwing unexpected exceptions. Just know that's a possibility and you may want to catch them and/or re-raise them depending on your use case.


Upload image with GD library First of all I just started learning OOP.

My priority is to make safe code. I think my code is safe, because I use GD to make a copy of the uploaded image and I use an image proxy to make sure nobody can access the uploaded file directly and it's uploaded outside of the webroot. But if I was 100% sure that it's safe I was not posting it here haha. I'm not sure about the way I check the MIME type, and about the GD.

I made some tests and it works fine, but what do you guys think? Any suggestions are welcome. Also if you have any questions about the code just ask me in the comments.

Class Img:

class Img
{
    private static $folder, $file, $ext, $w, $h;

    public static function upload($file, $new_size){
        self::$ext = pathinfo($file["name"], PATHINFO_EXTENSION);
        list(self::$w, self::$h) = ($size = getimagesize($file["tmp_name"]));

        if(!$size || !in_array(self::$ext, ['png', 'jpeg', 'jpg']))die(json_encode(['error' => 'Invalid file']));
        if(filesize($file["tmp_name"]) > 2700000)die(json_encode(['error' => 'File is too big']));

        self::$folder = dirname(__DIR__).'/../../uploads/profile/'.$_SESSION['user_folder'];
        self::$file = $file["tmp_name"];

        $_SESSION['avatar'] = ($name = mt_rand(100, 100000).'-'.time().'.'.self::$ext);
        self::resize_image($name, $new_size, $new_size);

        $user_id = $_SESSION['user_id'];
        $conn = \lib\Db::instance();
        $query = $conn->query("UPDATE users SET avatar = '{$name}' WHERE user_id = '{$user_id}'");
        $conn = NULL;

        die(json_encode(['avatar' => $_SESSION['avatar']]));
    }

    private static function resize_image($name, $width, $height){
        if(self::$ext === 'jpeg')self::$ext = 'jpg';

        if(self::$ext === 'jpg')$img = imagecreatefromjpeg(self::$file);
        elseif(self::$ext === 'png')$img = imagecreatefrompng(self::$file);

        $ratio = max($width/self::$w, $height/self::$h);
        $x = (self::$w - $width/$ratio)/2;
        self::$h = $height/$ratio;
        self::$w = $width/$ratio;

        $new = imagecreatetruecolor($width, $height);

        // preserve transparency
        if(self::$ext === "png"):
            imagecolortransparent($new, imagecolorallocatealpha($new, 0, 0, 0, 127));
            imagealphablending($new, false);
            imagesavealpha($new, true);
        endif;

        imagecopyresampled($new, $img, 0, 0, $x, 0, $width, $height, self::$w, self::$h);

        if(self::$ext === 'jpg')imagejpeg($new, self::$folder.$name);
        elseif(self::$ext === 'png')imagepng($new, self::$folder.$name);
        imagedestroy($new);
    }
}

avatar_upload.php:

require_once dirname(__DIR__).'/../../vendor/autoload.php';

if(!isset($_SESSION))session_start();
if($_SERVER['REQUEST_METHOD'] != 'POST' || empty($_SESSION['user_id']))die(header('HTTP/1.1 404 Not Found'));
if(!empty($_FILES['avatar']['size']))\lib\Img::upload($_FILES["avatar"], 150);

Fight the urge to squeeze multiple "things" into one line.  Your script will be easier to read and maintain with all declarations and constructs occupying their own rightful place.  Spend the extra lines, you'll be happy you did a year from now.
Use curly braces to encapsulate language constructs (e.g. if-else and foreach, etc.), this combined with appropriate tabbing will make your script easier to read and help to prevent mishaps when expressing conditional outcomes.  Start but don't stop reading about coding standards here: https://blog.sideci.com/5-php-coding-standards-you-will-love-and-how-to-use-them-adf6a4855696
Instead of murdering your script with die() calls, return a consistent data type no matter the outcome.  In the future, you may change how you want to display/deliver the results. Finally, convert your returned array to json and echo.
When writing variables into a query, you should be using a prepared statement  with placeholders and bound variables.
Avoid single-use and unnecessary variable declarations.  If you have $_SESSION['avatar'] then you don't need to declare $name.
Starting your session is not a conditional thing.  Do it early and do it every time.
You might like to ask yourself why you've elected to write static methods.  This is a topic worth investing some research time into.  Start, but don't stop here: When should I use static methods?

There is more to tell you, but I have run out of time.


Query Rails 3 Active Record Fetch associated model based on conditions Here is what I have. A User model and an Address model. User has_many :addresses and Address belongs_to :user.

Now I want to search for users whose email contains a give term, or one of the user's addresses contains that term. So lets say the term is 'jap', any User whose email like '%jap%' or any user whose any address city or address_line is like '%jap%' should be returned.

I can get the Users with a simple join like this:

users = User.joins('LEFT OUTER JOIN addresses ON users.id=addresses.user_id').where('users.email like '%jap%' OR addresses.city like '%jap%' OR addresses.address_line like '%jap%')

then to get those addresses (if any) that matched the search term I have to query again for each user returned:

users.each do |u|
    u.addresses.where("addresses.city like '%jap%' OR addresses.address_line like '%jap%'")
end

Is there a way to improve this so that I only search the addresses table once? I mean, a single query that returns the Users and the matching addresses for each user.

User.joins(:addresses).where(["addresses.city like ? OR addresses.address_line like ?", '%jap%', '%jap%'])

or

a = Address.arel_table
User.joins(:addresses).where(a[:city].matches('%jap%').or(a[:address_line].matches('%jap%')))


Elegant way to replace substring in a regex with optional groups in Python? Given a string taken from the following set:

strings = [
    "The sky is blue and I like it",
    "The tree is green and I love it",
    "A lemon is yellow"
]

I would like to constuct a function which replaces subject, color and optional verb from this string with others values.

All strings match a certain regex pattern as follow:

regex = r"(?:The|A) (?P<subject>\w+) is (?P<color>\w+)(?: and I (?P<verb>\w+) it)?"

The expected output of such function would look like this:

repl("The sea is blue", "moon", "white", "hate")
# => "The moon is white"

Here is the solution I come with (I can't use .replace() because there is edge cases if the string contains the subject twice for example):

def repl(sentence, subject, color, verb):
    m = re.match(regex, sentence)
    s = sentence
    new_string = s[:m.start("subject")] + subject + s[m.end("subject"):m.start("color")] + color
    if m.group("verb") is None:
        new_string += s[m.end("color"):]
    else:
        new_string += s[m.end("color"):m.start("verb")] + verb + s[m.end("verb"):]
    return new_string

Do you think there is a more straightforward way to implement this?

import re

regex = re.compile(
    r'(The|A) '
    r'\w+'
    r'( is )'
    r'\w+'
    r'(?:'
        r'( and I )'
        r'\w+'
        r'( it)'
    r')?'
)

def repl(sentence, subject, colour, verb=None):
    m = regex.match(sentence)
    new = m.expand(rf'\1 {subject}\2{colour}')
    if m[3]:
        new += m.expand(rf'\3{verb}\4')
    return new

def test():
    assert repl('The sky is blue and I like it', 'bathroom', 'smelly', 'distrust') == \
        'The bathroom is smelly and I distrust it'
    assert repl('The tree is green and I love it', 'pinata', 'angry', 'fear') == \
        'The pinata is angry and I fear it'
    assert repl('A lemon is yellow', 'population', 'dumbfounded') == \
        'A population is dumbfounded'

Essentially, invert the sections of the regex around which you put groups; they're the things you want to save.


Java String iterations I am working with some Java code. Basically I have a wrapper class that holds a string and implements some of the many useful python string methods in Java. My goal here is to implement the Python method .ljust and my hope is to be as efficient as possible. Currently, I am using a while loop which I imagine is terrible inefficient especially because it includes a +=. What's more, I am not sure which of the following alternatives is more efficient

String news = "";
news = this.toString();
int length = news.length();
while (length < size) {
        news += " ";
        length++;
}
return new PythonString(news);

In the above case, news.length() is only called once, but now there is an extra int and a new instruction length++.

The other option would be

String news = "";
news = this.toString();
while (news.length() < size) {
        news += " ";
}
return new PythonString(news);

In this case, news.length() is called every time.

I am interested in perspectives on which of these alternatives is most efficient as well as any other optimizations you could provide for this method, which is meant to pad a string with size spaces to the right so that all following characters line up at the same column.

The String class can do this already, see answer here: https://stackoverflow.com/a/391978/4217399. You can always look at the source of the String class to check how it's done, should be fairly optimized there, as it's a part of the JDK. It's actually offloaded to the java.util.Formatter class.


Write int, short int or any integer value in a file in binary form using fputc I am trying to write binary values in a file using fputc. In my case it is a short int.
The file must be opened in "w" mode.

I'm using this function, which is working. I need to know if I'm doing it right or if there are cases that I hadn't anticipated.

void writeListPos(Compressor* comp, short int listPos){
    ++listPos;
    char* posChar = (char*)(&listPos);
    int i;
    for(i=0 ; i<(sizeof(short int)) ; ++i){
        fputc(posChar[i] , comp->outputFile);
    }
}

The function to read the file later:

short int readListPos(Compressor* comp){
    char* posChar = (char*)(malloc(sizeof(short int)));
    int i;
    for(i=0 ; i<sizeof(short int) ; ++i){
        posChar[i] = comp->lastRead;/* comp->lastRead initialised with a first fgetc*/
        comp->lastRead = fgetc(comp->inputFile);
    }
    short int* positionp = (short int*)posChar;
    short int position = (*positionp);
    free(posChar);
    --position;
    return position;
}

Using malloc() and free() in readListPos() seems like overkill. You could use:

short s;
char *posChar = (char *)&s;

for (int i = 0; i < sizeof(s); i++)
{
    int c;
    if ((c = fgetc(comp->inputFile)) == EOF)
        …handle EOF…
    *posChar++ = c;
}
return s;

Note that this code handles EOF, unlike the original; you must always error check your inputs.

This code matches the code that writes the data.  It is, however, tied to a particular endian-ness; the written data is not portable between, say, an Intel machine and a SPARC or PowerPC machine.  If that doesn't matter to you, then you're more or less OK.

For non-portable reading and writing of binary data, the fread() and fwrite() functions are simpler.

Reading:

short s;
if (fread(&s, sizeof(s), 1, comp->inputFile) != 1)
    …process error/EOF…
return s;

Writing:

if (fwrite(&listPos, sizeof(listPos), 1, comp->outputFile) != 1)
    …process error…

If you're doing it portably, you define whether the number is written big endian or little endian and then write the bytes in that order on both big endian and little endian machines.  (If you can demonstrate it's a performance problem, you can optimize one of the two platform types, but it complicates the code for what is usually negligible benefit.  ('Premature optimization is the root of all evil.')


Test whether the edit-distance of two strings is at most 1 There are three types of edits that can be performed on strings: insert a character, remove a character, or replace a character.

EXAMPLES:

pale, ple returns true

pales, pale returns true

pale, bale returns true

pale, bake returns false

My solutions seems to work for any case I could think of (like ("", "s") but I feel like I'm checking conditions too much in it (mainly my final if/else statement had to be tacked on when I saw my solution wouldn't work for ("pales", "pale"))

bool oneAway(string s1, string s2)
{
    //loop through smaller string size
    decltype(s1.size()) size = (s1.size() < s2.size()) ? s1.size() : s2.size();

    for (decltype(s1.size()) i = 0; i < size; ++i)
    {
        if (s1[i] != s2[i])
        {
            string temp1 = s1.substr(i + 1);
            string temp2 = s2.substr(i + 1);

            //if rest of the string is equal we do 1 replacement.
            if (temp1 == temp2)
            {
                s1[i] = s2[i];
                break;
            }

            //otherwise we will try to insert or remove a character
            else
            {
                if (s1.size() < s2.size())
                {
                    s1.insert(i, 1, s2[i]);
                    break;
                }
                else
                {
                    s1.erase(i, 1);
                    break;
                }
            }
        }
    }

    //check equality
    if (s1 == s2)
        return true;

    //otherwise try to erase last character in s1 and check again (since for loop may not check this character if s2 was smaller string size)
    else
    {
        s1.erase(s1.size() - 1, 1);
        if (s1 == s2)
            return true;
    }
    return false;

}

This question is from the book "Cracking the Code Interview" by Gayle McDowell.

It looks like you either imported std::string or the whole standard namespace into the global namespace.
It's ok to do the former in implementation-files, though I would desist as it doesn't gain you all that much in brevity.
If it's the latter, read "Why is “using namespace std;” considered bad practice?" and change it.
Avoid allocating memory. Doing so is slow and can fail. That means accepting the arguments by constant reference, not using temporary copies, and not modifying the arguments.
If you had C++17, it would mean changing to std::string_view for the added flexibility.
Prefer using auto to a more complicated expression using decltype. It's less error-prone, more readable, and also shorter.
<algorithm> contains std::min(). Using that is more readable, shorter and no less efficient than writing it out using the conditional-operator.
Did you test ("abc", "b")? That's two deletions edit-distance, but will be accepted anyway.
Keep your line-length reasonable. Horizontal scrolling kills readability.

Using all that, but staying true to C++11:

#include <string>
#include <algorithm>

bool oneAway(const std::string& a, const std::string& b) noexcept {
    if (a.size() > b.size())
        return oneAway(b, a);
    if (b.size() - a.size() > 1) // No need to look further, pure optimization
        return false;
    auto begin = std::mismatch(a.begin(), a.end(), b.begin());
    using reverse_it = decltype(a.rbegin()); // std::make_reverse_iterator is C++14
    auto end = std::mismatch(a.rbegin(), reverse_it(begin.first), b.rbegin());
    return end.second.base() - begin.second < 2;
}

And for testing:

#include "one_away.h"
#include <iostream>
#include <cstdlib>

static bool do_test(const std::string& a, const std::string& b, bool expected) {
    bool r = oneAway(a, b);
    std::cout << (r == expected ? "[OK] " : "[FAIL] ")
        << std::boolalpha << r << " \"" << a << "\" \"" << b << \"\n";
    return r == expected;
}

static bool test(const std::string& a, const std::string& b = a, bool expected = true) {
    bool r = do_test(a, b, expected);
    if (a != b)
        r &= do_test(b, a, expected);
    return r;
}

int main() {
    bool r = test("");
    r &= test("abc");
    r &= test("pale", "ple");
    r &= test("pales", "pale");
    r &= test("pale", "bale");
    r &= test("pale", "bake", false);
    r &= test("abc", "b", false);
    std::cout << (r ? "[OK]\n" : "[FAIL]\n");
    return r ? 0 : EXIT_FAILURE;
}


Making a selection in a dropdown based on several possible values I'm making a Chrome extension that lets you choose a size of an item and check out quickly.

I have a value called "Large" in sizePref array in my chrome.storage. And when it calls that value, it will choose an option in a dropdown based on that. However, possible labels are "Large", "L", "M/L" or "L/XL" ("Large" being the most often) with the  value changing each time.

What is the most efficient way for me to ensure that it makes the correct selection (assuming the speed of doing so is the highest priority)?

This is my current code which just chooses "L" if "Large" doesn't exist. This code works without errors, but I want it to be improved.

chrome.storage.sync.get('sizePref', function(items) { // Get size preferences from storage
        var sizePref = items.sizePref.top1; // Set size to a var

        var sizeVal = $("#size option").filter(function() {
            return $(this).html() == sizePref;
        }).val();

        var sizeVal2 = $("#size option").filter(function() {
            return $(this).html() == "L";
        }).val();

        if (sizeVal !== undefined) {
            $("#size").val(sizeVal);
        } else {
            $("#size").val(sizeVal2);
        }

    });

The targeted dropdown code is as follows:

<select id="size" name="size"><option value="25243">Small</option>
<option value="25244">Medium</option>
<option value="25245">Large</option>
<option value="25246">XLarge</option></select>

If speed is important, then you should avoid unnecessary dom lookups.
For example,
if sizeVal is not undefined, then you'll never need another dom lookup for sizeVal2.
You can rearrange the code accordingly:

    var sizeVal = $("#size option").filter(function() {
        return $(this).html() == sizePref;
    }).val();

    if (sizeVal !== undefined) {
        $("#size").val(sizeVal);
    } else {
        var sizeVal2 = $("#size option").filter(function() {
            return $(this).html() == "L";
        }).val();
        $("#size").val(sizeVal2);
    }

I would go even further an cache the $("#size option"),
and also move the repeated logic into a function:

var options = $("#size option");

function findSize(target) {
    options.filter(function() { return $(this).html() == target; });
}

var size = findSize(sizePref).val() || findSize("L").val();
var $("#size").val(size);


Eratosthenes Sieve Implementation My friend and I were doing an Eratosthenes Sieve implementation, it works, but I think there is a mistake with "n" and with the first for condition ("i<=n"). I think that condition can cause a segmentation fault as there's no criba[n], am I right? Any suggestion to make it faster?

#include <bits/stdc++.h>

using namespace std;

const long long int n=1000000;
bool criba[n+1];

void gencriba(){
    memset(criba,true,sizeof(criba));
    criba[0]=criba[1]=false;
    for(long long int i=2;i<=n;++i){ ///Aquí es donde usa <=
        if(criba[i]){
            for(long long int j=i;j<=n/i;++j){
                criba[j*i]=false;
            }
        }
    }
}

int main()
{
    gencriba();
    int c;
    cin>>c;
    cout<<criba[c]<<endl;
    return 0;
}

Edit: Decided to change the array size to n+1, so that there's no possibility of getting an error because of invalid memory address

The code looks pretty good, but a few things that can get you into trouble should be addressed:

Don't #include <bits/stdc++.h>. See this answer on Stack Overflow for an explanation of why. TL;DR - it includes everything and you don't need all that it includes. It's just making your compile times take much longer than they should.

There's no need to use long long for values up to and including 1,000,000. A 32-bit long will do just fine with plenty of room in case you go over.

You should avoid global variables. In a program of this size it's no big deal, but it's a bad habit to get into. If your program grows to a larger size, it can become very difficult to reason about why and where a global variable is being changed, making it harder to find and fix bugs or to extend the application's functionality.

Be careful creating such large arrays. In this case, because they are global, they are in your program's data segment rather than on the stack so it's not an issue. However, if you were to create them on the stack in this manner, there is a chance that you could end up with a stack overflow error. 1 million bools will take up at least 1 Megabyte of memory. Many applications have a stack that is smaller than that, or only a little larger by default.

For readabliity, I recommend using more spaces in your code. In particular, I always leave a space before and after any operator. So your main() function would look more like this:

int main()
{
    gencriba();
    int c;
    cin >> c;
    cout << criba [ c ] << endl;
    return 0;
}

It makes it easier to see at a glance what's a variable and how they're being operated on.

In terms of performance, the best optimization is usually a better algorithm. This Stack Overflow Question asks which algorithms are fastest and has several answers.


Quick Sort Algorithm (Python) QuickSort is a Divide and Conquer algorithm, which picks an element as "pivot" and partitions a given list around the pivot. There are many different versions of Quick Sort, as to how to pick a pivot in different ways:

Always pick first element as pivot (implemented below)
Always pick last element as pivot
Pick a random element as pivot
Pick median as pivot

Just for practicing, I've implemented the quick sort algorithm, and if you'd like to review it and provide any change/improvement recommendations, please do so, and I appreciate that.

Code

import random
from typing import TypeVar, List
from scipy import stats

T = TypeVar('T')

def quick_sort(input_list: List[T]) -> List[T]:
    """"
    Returns an ascendingly sorted list;
    Input variable is an integer or float array;
    Theoretical Complexity: O(N*Log N) Time and O(N) Memory
    """

    sort(input_list, 0, len(input_list) - 1)
    return input_list

def sort(input_list: List[T], start_index: int, end_index: int) -> None:
    """Recursively sorts the two pivot-divided sublists;"""
    if start_index >= end_index:
        return
    pivot_index = partition(input_list, start_index, end_index)
    sort(input_list, start_index, pivot_index - 1)
    sort(input_list, pivot_index + 1, end_index)

def partition(input_list: List[T], start_index: int, end_index: int) -> int:
    """
    Returns the end index; Partitions a list into two sublists;
    """
    pivot = input_list[start_index]

    i, j = start_index + 1, end_index

    while i <= j:
        while input_list[i] < pivot and i < end_index:
            i += 1
        while input_list[j] > pivot:
            j -= 1

        if i < j:
            temp = input_list[i]
            input_list[i] = input_list[j]
            input_list[j] = temp
            i += 1
            j -= 1
        else:
            break

    input_list[start_index] = input_list[j]
    input_list[j] = pivot

    return j

if __name__ == "__main__":

    # Creates a dash line string and a new line for in between the tests.
    delimiter = "-" * 70 + "\n"

    # Generates a random integer list.
    test_list_integer = random.sample(range(-100, 100), 15) * 3
    print(f"""The unsorted integer array is:
        {test_list_integer}""")
    print(delimiter)

    # Generates a random float list.
    test_list_float = stats.uniform(0, 100).rvs(45)
    print(f"""The unsorted float array is:
        {test_list_float}""")
    print(delimiter)

    # Sample float/integer test list for input.
    integer_float_input = list(test_list_integer + test_list_float)

    # Sample float/integer test list for output.
    integer_float_output = sorted(integer_float_input)

    sorting_algorithms = [
        ("Quick Sort", quick_sort)
    ]

    # Testing
    for description, func in sorting_algorithms:
        if (func(integer_float_input.copy()) == integer_float_output):
            print(f"{description} Test was Successful.")
        else:
            print(f"{description} Test was not Successful.")
        print(f"""{description} (Integer):
            {func(test_list_integer.copy())}""")
        print(f"""{description} (Float):
            {func(test_list_float.copy())}""")
        print(delimiter)

Reference

Quick Sort - Geeks for Geeks
Quick Sort - Wiki

I'm all for providing docstrings.
I doubt the docstrings presented are helpful as can be for someone not familiar with the quicksort algorithm:

Returns an ascendingly sorted list;
is this a new one or the one provided as input?
(A method docstring should start with a phrase ending in a period [prescribing the] method's effect as a command ("Do this", "Return that"), not as a description)
Input variable is an integer or float array;
doesn't it work for strings?
the python tutorial names lists and tuples as standard sequence data types - what is an array?
(The type hint does give a hint as to the intended meaning - I do not have a helpful intuition about python type hinting in general and "interaction" with docstrings in particular.)
Theoretical Complexity: O(N×Log N) Time and O(N) Memory
Without further qualification of the bounds claimed, I expect them to be worst case bounds.
Recursively sorts the two pivot-divided sublists;
looking at the method interface, only, I don't see two sublists
(which happens to be the first thing I hope for in a method docstring - The docstring for a function or method should summarize its behavior and document its arguments, … does not stress this.)

There are several things not quite pythonic in the code presented

a swap of a and b is typically denoted using implied lists:
a, b = b, a
Concatenating immutable sequences always results in a new object of the same type as both sequences (the type of the first operand if not same types(?)) - not seeing the intention in enclosing test_list_integer + test_list_float in list()

I prefer

if not <condition>:
    <get out of here>
<operate>

over

if <condition>:
    <operate>
else:
    <get out of here>


URLify a string using Python I have been working my way through the exercises in the book Cracking the Coding Interview.  I wanted to get feedback for the following exercise.

  Write a method to replace all spaces in a string with '%20'.  You may assume that the string has sufficient space at the end to hold the additional characters and that you are given the true length of the string.

  Example:

  Input: "Mr_John_Smith____ ", 13
  Output: "Mr%20John%20Smith"

  Note: I am using '_' to represent spaces above because I could not get spaces to format nicely.

I would be interested in feedback on my code and time/space complexity computation below, and would also be interested to know if there is any way to improve the time or space complexity of my code.

def urlify(in_string, in_string_length):
    in_string = list(in_string)
    for position, character in reversed(list(enumerate(in_string[0:in_string_length]))):
        if character == ' ':
            in_string[position + 3:in_string_length + 2] = in_string[position + 1:in_string_length]
            in_string[position:position + 3] = '%20'
            in_string_length += 2
    return ''.join(in_string)

My guess is that the time complexity of the code above is \$O(1(n - s) +sn)\$ where \$n\$ is the length of the input string, \$ s\$ is the number of spaces in the input string (not including trailing spaces).

My reasoning is that if there are 0 spaces then the for loop will only do constant work checking the if statement and then perform the join at the end which all together is \$O(n)\$ whereas in the other extreme case where there are \$n\$ spaces, the work done will be \$O(n^{2})\$.  Lastly I think the space complexity is \$O(n).\$

Your complexity analyses look correct to me.  I would just say, in short, that this function works in O(n2) time.

Note that this is an awkward exercise to do in Python, or any language in which strings are immutable.  Since you can't perform the substitutions in place, you have to return a copy, which means that the minimum space requirement is O(n).  In that case, you might as well do either:

def urlify(in_string, in_string_length):
    return in_string[:in_string_length].replace(' ', '%20')

… or:

def urlify(in_string, in_string_length):
    return ''.join('%20' if c == ' ' else c for c in in_string[:in_string_length])

… which should also be O(n) space, O(n) time, and much simpler to implement!

That said, if we were to reinterpret the exercise to work on a buffer in place, I think that a better approach would be to make one pass to count the spaces in the input, to figure out the length of the output.  Then, copy characters to construct the result, working backwards from the end.  That algorithm should take O(n) time and O(1) space.

In summary, depending on how you interpret the exercise, the solution should either be simpler or faster than what you wrote.


Reporting success or failure of a child process The variable result is an integer representing the exit code of a command line process that was run via Groovy code. An exit code of 0 means the process was successful and an exit code from 1-255 means a failure. Then this line is executed:

println result ? "The command failed." : "The command succeeded."

What's happening here is that result is being coerced to a boolean (nonzero is true, zero is false) and then the ternary operator is used to determine which message to print, telling the user if the command succeeded or failed.

Is this fine in the context of the Groovy language, or is this going a bit too far with implicit magic?

In your original post, you wrote

The variable result is an integer representing the exit code of a command line process that was run via Groovy code. An exit code of 0 means the process was successful and an exit code from 1-255 means a failure. Then this line is executed:
println result ? "The command failed." : "The command succeeded."

I find it interesting you had to explain it so much.  I imagine if this were real code you may have to attach a similar code comment.
Personally I prefer to write code that requires no comments, if possible, and no explanation, by using well-crafted symbols.  So for example if you wrote it as
boolean success = (exitCode == 0);
println success ? "The command succeeded." : "The command failed.";

...you wouldn't have to explain that a exit code of 0 indicates success, and it wouldn't require a second glance to figure out the reversed ternary expression.
Any compiler worth its salt will optimize out the working variable and emit exactly the same executable code, so the increased readability costs you nothing.


Python Long UTC DateTime String now = datetime.datetime.utcnow()
now.strftime('%d %b %Y %I:%M:%S.%f')[:-3] + now.strftime(' %p')

Is there a cleaner solution for this? I don't want the extra 000 to appear everytime in %f.

EDIT: Got some more possible combinations:

now.strftime('%d %b %Y %I:%M:%S.%f')[:-3] + (' PM' if now.hour > 11 else ' AM')
now.strftime('%d %b %Y %I:%M:%S.X %p').replace('X', str(now.microsecond / 1000))

Maybe, this is marginally cleaner, based on @Gareth's comment:

'{0:%d} {0:%b} {0:%Y} {0:%I}:{0:%M}:{0:%S}.{1:03d} {0:%p}'.format(now, now.microsecond // 1000)

The not so great part is treating now.microseconds differently from the rest. But since the vale is an integer in the range 0..999999, and you want the value divided by 1000, I don't see a way around. For example if the value is 12345, you would want to get 012. Padding with zeros is easy, once the number is divided by 1000. Or you could pad with zeros up to 6 digits and take the first 3 characters, but I couldn't find a formatting exceptions to do that.

The advantage of this way over the original is instead of 2 calls to strftime and a string concatenation and a string slicing, there's now a single .format call.


Forgotten zombie killers Zombies are questions with no upvoted answers.
I came up with this SEDE query to find answers with zero votes on questions that have no accepted answer and no answers with greater than zero score.
In other words, answers on questions that show up on the Unanswered tab,
until the answer receives an upvote. (Latest version is here, FYI, not subject for review.)

DECLARE @username AS NVARCHAR(60) = ##DisplayName:string? ##;
DECLARE @userId AS INT = ##UserId:int?-1##;

SELECT TOP 100
  u.Id AS [User Link],
  a.Id AS [Post Link],
  a.CreationDate
FROM Posts a
  JOIN Users u ON a.OwnerUserId = u.Id
  JOIN (
    SELECT q.Id, MAX(a.Score) MaxAnswerScore
    FROM Posts q
    JOIN Posts a ON a.ParentId = q.Id
    WHERE q.AcceptedAnswerId IS NULL
    GROUP BY q.Id
  ) q
  ON a.ParentId = q.Id
WHERE
  (@username = '' OR u.DisplayName = @username)
  AND (@userId = -1 OR u.Id = @userId)
  AND a.Score = 0
  AND q.MaxAnswerScore = 0
ORDER BY a.CreationDate DESC

Is there a better way to write this?
Can it be improved?

This query looks very nice and clean. I can't see any obvious or glaring issues here, I do have some ideas for improvement though:
Naming
a, u, q and p are not all that "expressive" aliases. There is no need to shorten these aliases to be incomprehensible for people outside the SE-System, that don't understand how Posts a is different from Posts q
Select X
How about also making the number of results variable?
SELECT TOP ##topX:int?100##

allows the user to decide how many results they want.
CTE's
Last time I checked, Data Explorer supported CTE's. Make use of them:
WITH Zombies AS (
     SELECT q.Id ZombieId, MAX(a.Score) MaxAnswerScore
     -- ...
)

SELECT ##topX:int?100## FROM Zombies


Throttle actions by number per period I want to throttle asynchronous actions at a specific maximum rate of n actions per t period, and have pending actions wait until there is a free slot (rather than drop them). I want to receive the outcome When an action completes or fails.  I also want the option of cancelling an action.  The order of actions is unimportant, though I have been considering if a priority queue would be worth the trouble.

For example, if your maxActions was 2 and your maxPeriod 1 second, if 3 actions each taking 500 ms were queued simultaneously then the first two would run immediately and complete in 500ms but the 3rd would have to wait a whole second before starting, rather than starting at 500ms.

What I've come up with so far is based around two semaphores, one to control the number of actions active at any one time, and one to control the number of actions throughout the max period.  I release separately, because it is possible that a request may take longer than the given period.

Are there any logical errors to this approach (e.g., could a semaphore potentially never be released)? Is there a completely different approach that would be better?

public class Throttle
{
    private readonly TimeSpan _maxPeriod;
    private readonly System.Threading.SemaphoreSlim _throttleActions, _throttlePeriods;

    public Throttle(int maxActions, TimeSpan maxPeriod)
    {
        _throttleActions = new System.Threading.SemaphoreSlim(maxActions, maxActions);
        _throttlePeriods = new System.Threading.SemaphoreSlim(maxActions, maxActions);
        _maxPeriod = maxPeriod;
    }

    public Task<T> Queue<T>(Func<T> action, System.Threading.CancellationToken cancel)
    {
        return _throttleActions.WaitAsync(cancel).ContinueWith<T>(t =>
        {
            try
            {
                _throttlePeriods.Wait(cancel);

                // Release after period
                // - Allow bursts up to maxActions requests at once
                // - Do not allow more than maxActions requests per period
                Task.Delay(_maxPeriod).ContinueWith((tt) =>
                {
                    _throttlePeriods.Release(1);
                });

                return action();
            }
            finally
            {
                _throttleActions.Release(1);
            }
        });
    }
}

private readonly TimeSpan _maxPeriod;
private readonly System.Threading.SemaphoreSlim _throttleActions, _throttlePeriods;

I would add using System.Threading; at the top of the code file, so as to avoid having to fully qualify everything you're referencing in that namespace.

Also, at a glance (especially with the fully qualified SemaphoreSlim) it looks like you're declaring two readonly fields here, but there's really 3. I find this requires less brain juice to glance at:

private readonly TimeSpan _maxPeriod;
private readonly SemaphoreSlim _throttleActions;
private readonly SemaphoreSlim _throttlePeriods;

That said I like that the fields are readonly and that you're using an underscore prefix, which avoids having to qualify the fields with this in the constructor:

public Throttle(int maxActions, TimeSpan maxPeriod)
{
    this.throttleActions = new SemaphoreSlim(maxActions, maxActions);
    this.throttlePeriods = new SemaphoreSlim(maxActions, maxActions);
    this.maxPeriod = maxPeriod;
}

...and the reason you would have to qualify the fields in the constructor without that underscore prefix, is because you have a consistent naming scheme for your constructor parameters vs. the fields they're assigning, and that's excellent.

I'm not very familiar with asynchronous programming, multithreading or even the TPL - but Throttle tickles my Name-O-Meter™, as it's a verb more than a noun.. and class names should be nouns. Would Throttler be weird for a class like this? The only time a verb seems fine to me is when the class is static, such as Convert.

public Task<T> Queue<T>(Func<T> action, System.Threading.CancellationToken cancel)

Here we have the opposite situation: you're using a noun where a verb would be more appropriate - Enqueue<T> sounds more like it.

I'm on the fence about these comments:

    // Release after period
    // - Allow bursts up to maxActions requests at once
    // - Do not allow more than maxActions requests per period

It seems to me that they wouldn't be needed if the constructor had XML comments for its parameters:

/// <summary>
/// Instantiates a new <c>Throttle</c> object
/// </summary>
/// <param name="maxActions">The maximum number of actions allowed per period.</param>
/// <param name="maxPeriod">The time span to use for delaying tasks.</param>
public Throttle(int maxActions, TimeSpan maxPeriod)
{
    this.throttleActions = new SemaphoreSlim(maxActions, maxActions);
    this.throttlePeriods = new SemaphoreSlim(maxActions, maxActions);
    this.maxPeriod = maxPeriod;
}

I've never used Task.Delay, so I'm not 100% on the actual meaning of maxPeriod - but you get the idea: not only XML documentation cleans up comments in the middle of your implementation, it's also picked up by IntelliSense and shown when you're writing the client code.


MVVM model letting the user select a directory in a testable manner My application needs to let the user select a folder from somewhere on their local machine. Below are the interface and model pair that I have written to let the user do so.

public interface IFolderPicker
{
    //  Methods
    //  =======

    string Show();
}

using Microsoft.WindowsAPICodePack.Dialogs;
using WinForms = System.Windows.Forms;

public class FolderPicker : IFolderPicker
{
    //  Constants
    //  =========

    private const string SelectATargetFolder = "Select a target folder";
    private const string CDrive = @"C:\";

    //  Variables
    //  =========

    private readonly bool isPlatformSupported;
    private readonly WinForms.FolderBrowserDialog xpFolderBrowserDialog;
    private readonly CommonOpenFileDialog vistaFolderBrowserDialog;

    //  Constructor
    //  ===========

    public FolderPicker()
    {
        isPlatformSupported = CommonFileDialog.IsPlatformSupported;

        xpFolderBrowserDialog = new WinForms.FolderBrowserDialog
        {
            Description = SelectATargetFolder
        };

        vistaFolderBrowserDialog = new CommonOpenFileDialog
        {
            Title = SelectATargetFolder,
            IsFolderPicker = true,
            DefaultDirectory = CDrive,
            AllowNonFileSystemItems = false,
            EnsurePathExists = true,
            Multiselect = false,
            NavigateToShortcut = true
        };
    }

    //  Deconstructor
    //  =============

    ~FolderPicker()
    {
        xpFolderBrowserDialog.Dispose();
        vistaFolderBrowserDialog.Dispose();
    }

    //  Methods
    //  =======

    public string Show()
    {
        if (isPlatformSupported)
        {
            return SelectWinVistaOrLaterFolder();
        }
        else
        {
            return SelectWinXPFolder();
        }
    }

    private string SelectWinXPFolder()
    {
        WinForms.DialogResult result = xpFolderBrowserDialog.ShowDialog();

        switch (result)
        {
            case WinForms.DialogResult.OK:
            case WinForms.DialogResult.Yes:
                return xpFolderBrowserDialog.SelectedPath;

            default:
                return null;
        }
    }

    private string SelectWinVistaOrLaterFolder()
    {
        CommonFileDialogResult result = vistaFolderBrowserDialog.ShowDialog();

        switch (result)
        {
            case CommonFileDialogResult.Ok:
                return vistaFolderBrowserDialog.FileName;

            default:
                return null;
        }
    }
}

My reasoning for this design is that now the 3 readonly variables can be mocked in unit tests via reflection, while this isn't an ideal way of doing it, it's better than nothing.

While I am looking for all feedback, my focus here is on the WinForms.FolderBrowserDialog and the CommonOpenFileDialog. They both implement the IDisposable interface and my concern is that I might not be implementing that properly?
I believe that I could make the IFolderPicker interface extend the IDisposable interface and then call the two .Dispose() methods from within the inherited Dispose method but I was hoping I could insulate the rest of my program from these two classes as much as possible - is disposing of them in the deconstructor like this also valid?

Originally I had both these classed being instantiated in their respective Select...() methods in a using block. But while also not being mock-able, it meant they were being newed up each time the method was called - is this current implementation better than that?

Review

You always instantiate inner dialogs for both platforms, even though one will never be called. Your platform will not change at runtime. Use the Lazy pattern to only instantiate the required dialog at first access.
Rather than returning the magic path null, I would opt to use a TryShow method returning a boolean whether a path got selected an an out parameter for the path.

Object Destruction

From Programming Guide: Destructors:

  The programmer has no control over when the finalizer is called
  because this is determined by the garbage collector. The garbage
  collector checks for objects that are no longer being used by the
  application. If it considers an object eligible for finalization, it
  calls the finalizer (if any) and reclaims the memory used to store the
  object.

Since your class is not sealed, derived classes can be made. These classes will not be able to control the order of finalisation code. The order of destructors is fixed as this snipped from the link shows:

protected override void Finalize()
{
    try
    {
        // Cleanup statements...
    }
    finally
    {
        base.Finalize();
    }
}

That's why you should implement the dispose pattern instead. It allows for more flexibility in flow and distinguishes between disposing managed vs unmanaged code.

Unit Tests

  My reasoning for this design is that now the 3 readonly variables can be mocked in unit tests via reflection, while this isn't an ideal way
  of doing it, it's better than nothing.

When writing code with testability in mind, you don't want to resort to reflection to mock private state of your classes. There are a couple of options available:

Make the private methods protected virtual. Mocking frameworks and derived classes are able to override the methods. Partial mocks could be used in this case.
Wrap both internal dialogs behind a custom interface, injected into the outer class. The class no longer has dependencies on native win and vista components and interfaces are easy to mock.


1 Producer, N Consumers working in parallel, each doing ~1/nth of a task I am not wondering about error checking (I will add that soon). I would instead like to know about correctness, efficiency and simplicity.

#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

#define  NTHREADS    7
#define  RUN_N_TIMES 37

pthread_cond_t      new_task_available  = PTHREAD_COND_INITIALIZER;
pthread_cond_t      task_complete       = PTHREAD_COND_INITIALIZER;
pthread_mutex_t     mutex               = PTHREAD_MUTEX_INITIALIZER;

volatile int producerCond           = 0;
volatile int consumerCond[NTHREADS] = {};
volatile int no_more_tasks          = 0;
volatile int mini_tasks_complete    = - NTHREADS;

void do_work(unsigned int* current_level){
  /* Some work done */
  ++*current_level;
}

void *threadfunc(void *parm){
  volatile int* thread_cond;
  unsigned int level = 0;
  thread_cond = (volatile int*)parm;
  while(!no_more_tasks){
    pthread_mutex_lock(&mutex);
    mini_tasks_complete++;
    if(mini_tasks_complete > 0)
      printf("Thread %u has completed %u units of work.\nIt was #%d to complete it's task this round..\n", \
        (unsigned int)pthread_self(), level, mini_tasks_complete);
    if(mini_tasks_complete == NTHREADS){
      producerCond = 1;
      pthread_cond_signal(&task_complete);
    }
    *thread_cond = 0;
    while (!*thread_cond) {
      pthread_cond_wait(&new_task_available, &mutex);
    }
    pthread_mutex_unlock(&mutex);
    if(no_more_tasks){
      return NULL;
    } else {
      do_work(&level);
    }
  }
  return NULL;
}

void reset_cond(int val){
  int i;
  for (i=0; i<NTHREADS; ++i)
    consumerCond[i] = val;
}

int main(int argc, char **argv)
{
  int                   i;
  pthread_t             threadid[NTHREADS];

  for(i=0; i < NTHREADS; ++i) {
    pthread_create(&threadid[i], NULL, threadfunc, (void*)&(consumerCond[i]));
  }

  while(mini_tasks_complete < 0){
    /* Do nothing */
  }

  printf("Waking up all waiting threads " "#RUN_N_TIMES" " times...\n");
  for(i = 0; i < RUN_N_TIMES; i++){
    pthread_mutex_lock(&mutex);
    mini_tasks_complete = 0;
    printf("New tasks available.\n");
    reset_cond(1);
    /* Wake them up */
    pthread_cond_broadcast(&new_task_available);
    producerCond = 0;
    while (!producerCond) {
      printf("Main waiting\n");
      /* Go to sleep */
      pthread_cond_wait(&task_complete, &mutex);
    }
    pthread_mutex_unlock(&mutex);
  }
  pthread_mutex_lock(&mutex);
  reset_cond(1);
  no_more_tasks = 1;
  printf("Go home everybody!\n");
  pthread_cond_broadcast(&new_task_available);
  printf("Wait for threads and cleanup...\n");
  pthread_mutex_unlock(&mutex);

  for (i=0; i<NTHREADS; ++i) {
    pthread_join(threadid[i], NULL);
  }

  pthread_cond_destroy(&new_task_available);
  pthread_cond_destroy(&task_complete);
  pthread_mutex_destroy(&mutex);

  printf("Main done\n");
  return 0;
}

Here are some comments on your program.  Note that I am not a threads expert.

Firstly, the code runs nicely.  I'm running OSX and it compiles cleanly and
executes correctly (as far as I can tell).  I was a little surprised that
threads wake up in the same order each time - in other words if thread 123456
is the first to complete the first job, then it seems to be the first to
complete each subsequent job.  This says more about the thread implementation
than your code I guess.

The problem I have with the code is that although it seems to work, it is coded
very specifically for the problem in hand.  The barriers ensuring that all
threads come together at intervals are not separately identifiable but are
entwined in the control code.  The variable mini_tasks_complete, used in
creating the barrier is initialized to -NTHREADS, allowed to run up to zero
during the initial barrier and then cycles from 0 to NTHREADS.  This clearly
works but is inelegant (I think you will find that the initial barrier can be
omitted and worked into the loop with a bit of thought).

I would prefer to see the thread function have the simple form:

while (more_tasks) {
    barrier();
    work();
}

and the main thread have the general form:

for (...) {
    prepare_work();
    barrier();
}

Written like this one can clearly identify the synchronisation.  The barrier function
here would also preferably take a parameter identifying the barrier and not
use globals.

I assume your platform, like mine, lacks the pthread barrier.  You might write
your own barrier function.  From what I have read, barriers are normally based
upon a pair of condition variables and a pair of mutexes.  The Little Book of
Semaphores has a discussion of this and there is a useful Solaris
implementation that could be used as a template.

Some trivia:

I would find while (more_tasks) more logical than while (!no_more_tasks)
inconsistent use of camelCase


Efficiently consolidating an array of objects In my program, I'm getting Facebook objects and using temp to display on a separate screen how many unread messages they have. However, we have since decided to consolidate similar messages, such as getting XP from a Facebook friend more than once. We want those messages to be one message with the sum of the XP gifted from the friend. That part is fine and done. But, this makes temp an inaccurate representation, because the program will show how many Facebook objects there are instead of how many consolidated messages

I have written this code to loop through the objects and get a count of how many there are (if there is an ill-formatted object, we don't want to count it so array.length is out of the question). Then I am looping through the array again TWICE to check items in the array against other items. If they are consolidated, I want the array to behave as if the objects weren't there, because removing items from an array while you're looping through it can cause problems and this seemed the easiest way.

Is there a way to do this more efficiently, perhaps in fewer loops?

var consolidated:Array = [];

for (var i:int = 0; i < requests.length; i++)
            {
                var dataJ:Object = JSON.parse(requests[i].data);

                if (dataJ.title == "ask")
                {
                    //check for duplicates
                    if (dupeCache[requests[i].from.id] == undefined)
                    {
                        dupeCache[requests[i].from.id] = true;
                        temp++;
                        consolidated.push(requests[i]);
                    }
                }
                else if (dataJ.title == "gift")
                {
                    temp++;
                    consolidated.push(requests[i]);
                }

            }
            var popped:int = 0;
            for (var j:int = 0; j < consolidated.length; j++)
            {
                var dataObjectJ:Object = JSON.parse(consolidated[j].data);
                popped= 0;
                for (var k:int = 0; k < consolidated.length; k++)
                {
                    var dataObjectK:Object = JSON.parse(consolidated[j].data);

                    if (consolidated[j] == consolidated[k]) continue;

                    if (consolidated[j].from.id == consolidated[k].from.id)
                        if (dataObjectJ.title == dataObjectK.title)
                            if(dataObjectJ.type == dataObjectK.type)
                            {
                                //we dont want to consolidate hearts
                                if (dataObjectJ.type == "heart") continue;
                                temp--;
                                popped++;
                            }
                }
                j += popped;

            }

            globals.blob.unreadMessageCount = temp;

I'm afraid I'm not familiar with Actionscript but I can give you some general advice with regards to how your code is structured:

Separate the requests into different arrays for each title instead of keeping them in a single array. Admittedly this is a violation of the DRY principle (don't-repeat-yourself) but it will force you to apply specific handling rules for each rather than a general rule that you bend that you do at the moment. This is clear from the degree of nesting of if-statements in your code.
You seem to JSON.parse(...) twice, can you not keep the result from the original call?
You seem to check the uniqueness of the sender id twice requests[i].from.id and consilidated[j].from.id. I think you only need to do this once?
Try to rely on array lengths rather than counters that you implement yourself. I know you've commented on it but consider building and modifying separate arrays.
Don't nest your if-statements but create separate boolean variables.

var idIsEqual:Boolean = consolidated[j].from.id == consolidated[k].from.id;
var titleIsEqual:Boolean = dataObjectJ.title == dataObjectK.title;
var typeIsEqual:Boolean = dataObjectJ.type == dataObjectK.type;
if (idIsEqual && titleIsEqual && typeIsEqual) {
    ...
}

It might be a little overkill but consider creating an associative array keyed by id_title_type (e.g. "12_myTitle_gift"). That would make searching a little easier.
Don't create variables named temp. This is a real bugbear of mine and I see it all the time. By the looks of it, it should probably be named unreadMessageCount. For someone who doesn't know you code, the penny doesn't really drop on what your code is trying to achieve until the last line is read, and suddenly everything becomes clear.


Would this linked list be considered clean if written in an hour-long interview? If you were interviewing me, and saw this code written in a little under an hour, would you consider me as a candidate for a position at your company if you knew I had 2 years of C++ experience?

#include <stdlib.h>
#include <cassert>

/*! \class Node to be used in likned list like constructors
 */
template <typename T>
class node
{
private:
  T data;
  node * next;

public:
  /*! \brief Default constructor
   * Initialize next pointer to NULL and let data's default constructor
   * be called
   */
  node() :
    next(NULL)
  {}

  /*! \brief Parametered constructor
   * Initialize next pointer to NULL and set data to t
   */
  node(T t) :
    data(t),
    next(NULL)
  {}

  /*! \brief Copy constructor
   */
  node(const node & other) :
    data(other.data),
    next(other.next)
  {}

  /*! \brief Destructor
   */
  ~node()
  {}

  /*! \brief Assignment operator
   */
  node & operator=(const node &other)
  {
    data = other.data;
    next = other.next;

    return (*this);
  }

  /*! \brief Set the next pointer
   */
  void set_next(node *next_node)
  {
    next = next_node;
  }

  /*! \brief Set the nodes data
   */
  void set_data(T t)
  {
    data = t;
  }

  /*! \brief Get the nodes data
   */
  T get_data() {
    return data;
  }

  /*! \brief Get the nodes next pointer
   */
  node * get_next()
  {
    return next;
  }

  /*! \brief Equivalence operator
   */
  bool operator==(const node &other)
  {
    return (data == other.data && next == other.next);
  }

  /*! \brief Not equivalence operator
   */
  bool operator!=(const node &other)
  {
    return (data != other.data || next != other.next);
  }
};

/*! \class Linked List
 */
template <typename T>
class linked_list
{
private:
  node<T> * head;

public:
  /*! \brief Default constructor
   * Initialize head pointer to NULL
   */
  linked_list() :
    head(NULL)
  {}

  /*! \brief Parametered constructor
   * Initialize head to a node
   */
  linked_list(node<T> &n) :
    head(&n)
  {}

  /*! \brief Copy constructor
   */
  linked_list(const linked_list & other) :
    head(other.head)
  {}

  /*! \brief Destructor
   */
  ~linked_list()
  {}

  /*! \brief Assignment operator
   */
  linked_list & operator=(const linked_list &other)
  {
    head = other.head;

    return (*this);
  }

  /*! \brief Get the head node pointer
   */
  node<int> * get_head()
  {
    return head;
  }

  /*! \brief Equivalence operator
   */
  bool operator==(const linked_list &other)
  {
    return (head == other.head);
  }

  /*! \brief Not equivalence operator
   */
  bool operator!=(const linked_list &other)
  {
    return (head != other.head);
  }
};

void unit_test_linked_list()
{
  // Default constructor
  linked_list<int> l0;
  assert(!l0.get_head());

  // Parametered constructor
  node<int> n(5);
  linked_list<int> l1(n);
  assert(l1.get_head()->get_data() == 5);

  // Copy construtor
  linked_list<int> l2(l1);
  assert(l2 == l1);

  // Assignment and equivalence
  l0 = l2;
  assert(l2 == l1);
  assert(l2 == l0);
  assert(l1 == l0);
}

void unit_test_node()
{
  // Default constructor
  node<int> n;
  assert(!n.get_next());

  // Parameterd constructor and assignment operator
  n = node<int>(5);
  assert(!n.get_next() && n.get_data() == 5);

  // Copy constructor equivalence operator and not equivalent operator
  node<int> m(n);
  assert(n.get_data() == m.get_data() && n.get_next() == m.get_next());
  assert(!(m != n) && m == n);

  // Setters and getters
  m.set_data(6);
  m.set_next(&n);
  assert(m.get_data() == 6 && m.get_next()->get_data() == 5);
  assert(m.get_next()->get_next() == NULL);
}

int main()
{
  unit_test_node();
  unit_test_linked_list();
}

I can't speak for others, but I'd personally have some reservations. To be fair, perhaps I'm not being lenient enough for doing this under pressure in an hour, but there are multiple issues that I'd want to address with this code.

Questions and Small Problems

Why is node not an internal class of linked_list? Generally, the user should neither know nor care about exactly how a node is represented. I'd let this slide pretty easily though, because it's a small oversight and can be fairly easily corrected.

You're not passing to your node constructor by const T& which I would have expected. If this is constructing from a type that is expensive to copy construct, this adds quite a bit of overhead.

Usage of NULL vs nullptr. Not a big deal, but it'd make me push you a little bit on knowledge of C++11 constructs.

Pain Points

The interface for your list is (somewhat) setup, but the implementation is very, very incomplete. As it currently stands, it doesn't really represent a list at all, but something that holds the address of a single element that exists outside the list, and is therefore tied to the lifetime of that object (which is a big, big problem).

Your linked_list constructor takes a node<T> as a parameter. This goes back to the first point about node being internal, but here it is more serious in my mind. This shows that you do expect the user to know/care about the node type. It also takes it by reference, and then simply makes the head node point to that address. This is all a bit worrying. It should be taking an element of type T, which it then wraps up in a node, and that is stored as the head element:

linked_list(const T& initial)
  : head(new node(initial))
{ }

You never new or delete anything - a linked_list that uses absolutely no dynamic storage is very strange indeed. It'd certainly make me want to press you on things like object lifetimes, stack and heap allocation, dangling references and the like.

There is no way to modify a list once it is created; something as basic as adding an element isn't possible. Even if these functions weren't finished in time, I'd expect just the declarations to be there, something like:

void push_front(const T& data)
{ }

Because of the lack of dynamic allocation, your copy constructor and assignment operator don't really do anything, and you don't have a destructor. Implementations of these would be something I'd heavily scrutinize. For a start, do they do what they are supposed to do? A copy constructor should be doing a deep copy of the list it is passed - many people get this wrong and end up simply doing a shallow copy, which generally means pointers pointing to deleted memory, and undefined behaviour.

operator= should either be using the copy-swap idiom or at least doing basic checks like (this != &rhs), clearing the current data in the list, and copying the data from what is passed in as the argument to operator=. With your current implementation, none of this can be demonstrated properly.

Why is get_head declared as node<int> *? Why are you returning a node instead of simply returning the data? This should have two overloads, one for const T& and one for T&.

Operators shouldn't be member functions, but that's pretty minor, and I wouldn't really care too much. However, if you do make them member functions, they should definitely be const:

bool operator!=(const linked_list &other) const
                                        //^^^^^

All in all, there is a lot lacking for an implementation, even given the short time span. The lack of any kind of dynamic memory allocation would probably be a deal-breaker for me personally. Without being able to hear your reasoning about this, I'd make the assumption that you didn't understand allocations or object lifetimes, which is of critical importance when programming in C++.

Edit: If given a limited time (such as one hour) to write something like a list, I'd probably approach it in the following way: firstly, sketch out the minimal interface I want to develop from. Given the pretty strict time requirements, I'd keep it as simple as possible:

A private internal node struct with all public members (this cuts down on boilerplate code that needs to be written, and since it's private, it's not user facing, so I'd argue it's fine anyway).
Private members for list, which I'd keep to a node *head and std::size_t size.
Basic constructors, copy assignment, copy constructor, destructor.
Insert, update and delete. Since this is a singly linked list, insert would be limited to adding something to the front (push_front if we're following standard lib naming), something to update a given value, and something to delete a given value. Usually these would use iterators, but that's too complex given the tight time restrictions.
Something to get the size, if the list is empty, and maybe some basic comparison operators.

Sketching out this API, I'd have something like:

template <typename T>
class linked_list
{
private:

    struct node
    {
        T data_;
        node* next_;

        node(const T& data)
          : data_(data),
            next(nullptr)
        { }
    };

    std::size_t size_;
    node* head_;

public:

    linked_list()
      : head_(nullptr),
        size_(0)
    { }

    linked_list(const T& data)
      : head(new node(data)),
        size_(1)
    { }

    ~linked_list() { }
    linked_list(const linked_list& rhs) { }
    linked_list& operator=(linked_list rhs) { } //Use copy-swap idiom

    bool is_empty() const { }
    std::size_t size() const  { }

    void push_front(const T& value) { }
    bool remove(const T& value) { }
    bool update(const T& value) { }

    void swap(linked_list& rhs) { }
};

template <typename T>
void swap(linked_list<T>& lhs, linked_list<T>& rhs) { }

template <typename T>
bool operator<(const linked_list<T>& lhs, const linked_list<T>& rhs) { }

template <typename T>
bool operator>(const linked_list<T>& lhs, const linked_list<T>& rhs) { }

template <typename T>
bool operator==(const linked_list<T>& lhs, const linked_list<T>& rhs) { }

template <typename T>
bool operator!=(const linked_list<T>& lhs, const linked_list<T>& rhs) { }

I'd then start to fill in functions as I went - some of them are one-liners (is_empty and size), some of them take a bit more work. I'd pay the most attention to getting the destructor and copy constructor right, since these are probably the functions that would be scrutinized the most. Even if this API is slightly ambitious, and if some of the functions don't end up being implemented, at least it shows intent.

Finally, after the hour was up, I'd want to be ready to discuss what this implementation is lacking compared to say, the standard library list: from memory, Allocator template parameter, iterator + const_iterator, typedefs such as value_type, reference_type, functions such as remove_if, sort, unique and so on (although this is probably getting to in-depth - unless you've been working with the standard library a lot and know it very well, this is probably too much detail - but it might be worth it to know at least a little bit about std::list).

Given the incredibly limited time, I'd likely eschew unit testing. In fact, unless I had a compiler, I'd be almost positive there would be at least a few errors lurking around - syntax or otherwise. This wouldn't bother me - few people can sit down and bang out even a simplified container class without making a single mistake. To me, it is the high level overview that is important, along with those few key functions (destructor, copy constructor).


Reading all bytes from a file I'm basically trying to write a helper function that reads a whole file and returns the data and the number of bytes read.

Can you tell me if is correctly written and used?

#include <iostream>

static char * ReadAllBytes(const char * filename, int * read)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();
    int length = pos;
    char *pChars = new char[length];
    ifs.seekg(0, ios::beg);
    ifs.read(pChars, length);
    ifs.close();
    *read = length;
    return pChars;
}

int _tmain(int argc, _TCHAR* argv[])
{
    const char * filename = "polar00.map";
    int read ;
    char * pChars = ReadAllBytes(filename, &read);
    delete[] pChars;
    return 0;
}

A few things I would do differently:

static char * ReadAllBytes(const char * filename, int * read)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();

    // What happens if the OS supports really big files.
    // It may be larger than 32 bits?
    // This will silently truncate the value/
    int length = pos;

    // Manuall memory management.
    // Not a good idea use a container/.
    char *pChars = new char[length];
    ifs.seekg(0, ios::beg);
    ifs.read(pChars, length);

    // No need to manually close.
    // When the stream goes out of scope it will close the file
    // automatically. Unless you are checking the close for errors
    // let the destructor do it.
    ifs.close();
    *read = length;
    return pChars;
}

How I would do it:

static std::vector<char> ReadAllBytes(char const* filename)
{
    ifstream ifs(filename, ios::binary|ios::ate);
    ifstream::pos_type pos = ifs.tellg();

    std::vector<char>  result(pos);

    ifs.seekg(0, ios::beg);
    ifs.read(&result[0], pos);

    return result;
}

Note:

static std::vector<char> ReadAllBytes(char const* filename)

It may seem like an expensive copy operation. But in reality NRVO will make this an in-place operation so no copy will take place (just make sure you turn on optimizations). Alternatively pass it as a parameter:

static void ReadAllBytes(char const* filename, std::vector<char>& result)


Number pyramid in Java The function func(int m,int n) outputs (for m = 3, n = 5):

3

34

345

34

3

I came up with this code:

void func(int m,int n)
{
  for(int i=1;i<n-m+2;i++)
{   int k=i;
  int j=m;

    while(k>0)
  {
    System.out.print(j);
     k--;j++;
   }
 System.out.println("");

}

for(int i=n-m;i>0;i--)
{  int k=i;
  int j=m;

  while(k>0)
  {
  System.out.print(j);
     k--;j++;
   }
System.out.println("");

    }

}

Is there an alternate, better way to do it? Can it be more efficient?

You can do it recursively:

void func(int m,int n) {
  if (m == n) {
    System.out.println(m);
  } else {
    func(m, n - 1);
    for (int i = m; i <= n; i++) {
      System.out.print(i);
    }
    System.out.println("");
    func(m, n - 1);
  }
}


Different method to calculate blob area in OpenCv I'm trying to use a different method to calculate a blob area from contourArea, using the countNonZero function. I want to see if there is significant approximation introduced by contourArea function, and also be able to calculate area of contour with auto-intersection.

cv::findContours(drawingIn, contoursIn, CV_RETR_TREE, CV_CHAIN_APPROX_NONE);

double max=0;
double currentarea=0;
int currentareaCounted = 0;
int maxAreaCounted = 0;

for (unsigned int i=0;i<contoursIn.size();i++)
{
    currentarea=cv::contourArea(contoursIn[i]);
    cv::Mat drawingMatCount = cv::Mat::zeros(ROI_INTERACTION,ROI_INTERACTION, CV_8UC1);
    cv::drawContours(drawingMatCount, contoursIn, i, 255, -1, 8);
    currentareaCounted = cv::countNonZero(drawingMatCount);

    if (currentarea>max)
        max=currentarea;
    if (currentareaCounted>maxAreaCounted)
        maxAreaCounted=currentareaCounted;

}

Is there something that could be improved? This code works, but my doubt is about declaring a new matrix every iteration. Could this lead to some performance issue?

Declare drawingMatCount once before the loop
cv::drawContours(drawingMatCount, contoursIn, i, 0, -1, 8);  at the end of the loop

BTW: this is not the blob area, its the contour area, including the holes.


Designing a Backspace key I have designed a Backspace button (part of a keyboard which I am designing) in windows forms and it works perfect with no errors (the code works very good). This button deletes what is inside the textbox.

I just want to see other people's views and what they think about my code, so because I am new to c# I think there might be other better ways to design Backspace button. Can you please see if there are any silly unprofessional mistakes in my code?

private void buttonDelete_Click(object sender, EventArgs e)
    {

        textBox1.Focus(); // set the textbox focused.

        if (textBox1.SelectionStart >= 0) //check if there is text to delete

        {
           int TxTindex = textBox1.SelectionStart; // save the index position

            if (textBox1.SelectedText.Length > 0)  //// check if there is selected text
            {

                textBox1.SelectedText = "";

            }

            else
            {

                if (TxTindex >0 && TxTindex == textBox1.Text.Length) // check if there is text in texbox and check if index is at the end of text.

                {

                    textBox1.Text = textBox1.Text.Remove(textBox1.Text.Length - 1, 1);
                    textBox1.SelectionStart = textBox1.Text.Length;

                }

                else // is the index is in between the text?
                {
                    if (TxTindex > 0) // check if there is text in texbox
                    {

                        textBox1.Text = textBox1.Text.Remove(TxTindex - 1, 1);
                        textBox1.SelectionStart = TxTindex - 1; // to set the cursor position after the deleted number between the text.
                    }
                    else
                    {

                        return;
                    }

                }

            }
        }
        else
        {

            return;
        }
    }

Through a series of mechanical transformations, it's possible to simplify the code down to this:

private void buttonDelete_Click(object sender, EventArgs e)
{
    textBox1.Focus();

    if (textBox1.SelectionLength > 0)
    {
        textBox1.SelectedText = "";
    }
    else if (textBox1.SelectionStart > 0)
    {
        textBox1.Text = textBox1.Text.Remove(--textBox1.SelectionStart, 1);
    }
}

It's now simple enough that the code would be better without comments.

How I got there:

Both of your else { return; } are superfluous.
The if (TxTindex >0 && TxTindex == textBox1.Text.Length) case is a redundant special case; its else clause contains exactly the same code.

The code so far:

private void buttonDelete_Click(object sender, EventArgs e)
{
    textBox1.Focus(); // set the textbox focused.

    if (textBox1.SelectionStart >= 0) //check if there is text to delete
    {
        int TxTindex = textBox1.SelectionStart; // save the index position

        if (textBox1.SelectedText.Length > 0)  //// check if there is selected text
        {
            textBox1.SelectedText = "";
        }
        else if (TxTindex > 0) // check if there is text in texbox
        {
            textBox1.Text = textBox1.Text.Remove(TxTindex - 1, 1);
            textBox1.SelectionStart = TxTindex - 1; // to set the cursor position after the deleted number between the text.
        }
    }
}

The outer if (textBox1.SelectionStart >= 0) is redundant, since the if (textBox1.SelectionText.Length > 0) and else if (TxtIndex > 0) tests are both more specific.
textBox1.SelectedText.Length is better written as textBox1.SelectionLength.
The TxtIndex variable (which would be better named textIndex to be consistent with textBox1) can be eliminated.


Reading a gzipped file with JSON data for thousands of users I'm a newbie in Python.

I need to read a GZ file contains thousands of users.
But I think my code is not optimized in this case. Could anyone can help me? What should I do to improve performance? Or is it standard?

if os.path.isfile("file.gz"):
    with gzip.GzipFile("file.gz", 'r') as fin:
        for line in fin:
            if get_new_user is True:
                if datetime.strptime(json.loads(line).get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == (datetime.today()-timedelta(1)).date():
                    users.append(json.loads(line))
            else:
                users.append(json.loads(line))
    os.remove("file.gz")
    return users

It looks like your code is somewhat non-optimal.

In the case get_new_user is True, you call json.loads(line) once for the "UpdatedAt" test, and if that passes, you call it again, returning effectively the same value, to append the users list.
In the case of get_new_user is True, you call (datetime.today()-timedelta(1)).date() once for each user, yet it is effectively constant.  You should move this out of the loop.

You could refactor your code like this:

threshold = (datetime.today()-timedelta(1)).date()
for line in fin:
    user = json.loads(line)
    if get_new_user is True:
        if datetime.strptime(user.get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == threshold:
            users.append(user)
    else:
        users.append(user)

You are doing the get_new_user is True test for each user/line in the file.  That too is constant, and only needs to be done once.  It is a tiny gain, but you could put the for loop inside the if and else clauses.

if get_new_user is True:
    threshold = (datetime.today()-timedelta(1)).date()
    for line in fin:
        user = json.loads(line)
        if datetime.strptime(user.get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == threshold:
            users.append(user)
else:
    for line in fin:
        users.append(json.loads(line))

And maybe using some list comprehensions and a generator expression for fun ...

if get_new_user is True:
    threshold = (datetime.today()-timedelta(1)).date()
    users = [ user for user in (json.loads(line) for line in fin)
              if datetime.strptime(user.get('UpdatedAt'),
                                   '%Y-%m-%dT%H:%M:%S.%fZ'
                                  ).date() == threshold ]
else:
    users = [ json.loads(line) for line in fin ]

Or if you are truly brave ... almost a 1-liner:

threshold = (datetime.today()-timedelta(1)).date()
users = [ user for user in (json.loads(line) for line in fin)
          if get_new_user is not True or
             datetime.strptime(user.get('UpdatedAt'),
                               '%Y-%m-%dT%H:%M:%S.%fZ'
                              ).date() == threshold ]

I've left your if get_new_user is True: statement alone, but it is a little non-Pythonic.  Usually, you'd just use if get_new_user:, but those two statements are very different.  The first is true if and only if get_new_user is True whereas the second would be true when get_new_user contains any non-falsey value, such as: True, or "True" or 1, or 17 or "False", or [0], ... etc.


Smallest letter in a string As part of my college class, I have to write a program that returns the "smallest" letter in the string. Input is assumed to be a non empty string. "Smallest" is defined as:

  The smallest decimal value of the character, pertaining to this chart.

The range of acceptable values is 0 <= x <= 127

I would like to get feedback on the algorithm of the code. Other suggestions are accepted and welcome, but the main algorithm is the main focus.

def smallest_letter(string: str) -> str:
    """
    Returns the smallest letter in the string

    Input is restricted to ASCII characters in range 0 <= character <= 127

    :param string: A string containing only ASCII letters

    :return: A string length one of the smallest letter
    """

    # Ensure all characters are within the acceptable range #
    for character in string:
        assert ord(character) <= 127

    smallest_letter = 1000
    for letter in string:
        if ord(letter) < smallest_letter:
            smallest_letter = ord(letter)
    return chr(smallest_letter)

if __name__ == "__main__":

    # Test Cases #

    assert smallest_letter("abydsufaksjdf") == "a"
    assert smallest_letter("bfsufsjfbeywafbiu") == "a"
    assert smallest_letter("ABYUVGDuibfsafuofiw") == "A"

Regarding min(string): Python exceeding your expectations. It happens a lot.

assert

All assert statements can be disabled with a switch to the interpreter, and sometimes are. Therefore, they're not suitable for flow control. I think you should replace:

for character in string:
    assert ord(character) <= 127

With:

for character in string:
    if ord(character) > 127:
        raise ValueError(f"Character {character} is out of range.")

or to optimize with a generator instead of the loop (requires Python 3.8+ if you want to report the character that violates the condition):

if any(ord(character := char) > 127 for char in string):
    raise ValueError(f"Character {character} out of range")

(Thanks to @Graipher for proper handling of the := token in 3.8+, which I hadn't worked with myself yet.)

That's all I can see being wrong here, though.


Grab first successful pair from a list of maps I have a function that parses geographic coordinates through a list of different providers. In this case I want to look up two addresses. The main problem of this question is - given a List of of Geocode Service -> Resolved Address mappings, find the first successful pair that has a valid resolved address.

So each address results in a map(provider -> result). I'm given a list of them, and need to find the a successful pair from the list e.g provider1 from the first address and provider1 from the second address. I can't compare the resolved addresses if the providers do not match.

For example, the best scenario is that the addresses are perfect. I can use the head map entry of the first address and query the map entry for the second address. Then compare their values. However, it's rarely that simple.

I started out with a foldLeft, but I feel that can be inefficient if it matches successfully immediately.

val result: List[Map[GeoProviders.GeoProvider, Either[GeoCodeErrors.GeocodeError, PostalAddress]]]
                  result match {
                    case firstAddressOption :: secondAddressOption :: Nil => {
                      firstAddressOption.foldLeft[Option[Double]](None)((result, address1: (GeoProvider, Either[GeocodeError, PostalAddress])) => {
                        result match {
                          case Some(_) => result
                          case None =>
                            val address2 = secondAddressOption.get(address1._1)
                            (address1._2, address2) match {
                              case  (Right(successAddr1), Some(Right(successAddr2))) => {
                                (successAddr1.coordinates, successAddr2.coordinates) match {
                                  case (Some(firstCoord), Some(secondCoord)) => {
                                    Some(haversineDistance((firstCoord.latitude, firstCoord.longitude), (secondCoord.latitude, secondCoord.longitude)))
                                  }
                                  case _ => None
                                }
                              }
                            }
                        }
                      })

You can use find to avoid folding over an entire map, something like:

map1.find {
  case (k, Right(addr1)) =>
    map2.get(k) match {
      case Some(Right(addr2)) => true
      case _ => false
    }
  case _ => false
} match {
  case Some((k, Right(addr1))) =>
    map2.get(k) match {case Some(Right(addr2)) => Some(haversineDistance(addr1, addr2))}
  case _ => None
}

(I renamed first/secondAddressOption to map1/map2 to save some typing)

An alternative approach, which doesn't necessarily improve performance but does make the code shorter and more reusable, is to create a method that combines two maps by key:

def combineMaps[K, V](map1: Map[K, V], map2: Map[K, V]): Map[K, List[V]] =
  // To improve performance, compare the sizes of map1 and map2
  // and fold over the smaller map.
  map1.foldLeft(map2.mapValues(List(_))) {
    case (map2, (k, x)) => map2.updated(k, x :: map2.get(k).toList.flatten)
  }

Then use collectFirst on the combined map:

combineMaps(map1, map2).collectFirst {
  case (_, Right(addr1) :: Right(addr2) :: Nil) => haversineDistance(addr1, addr2)
}


Book repository for storing books that are accessed by ISBN I want to implement a book repository using a map where books can be added, removed and updated. Books in this repository should be accessed by their ISBN which is an object property. The books should not be edited from the outside of the repository, because if a book’s ISBN is changed from the outside, the ISBN used as key is out of sync with the ISBN of the book.

Another option is to use a list, so I don’t have a key that can be out of sync.

I came up with the following solution.

Book:

public class Book {
    private String isbn;
    private String title;
    private String author;
    private double price;

    public Book(String isbn, String title, String author, double price) {
        this.isbn = isbn;
        this.title = title;
        this.author = author;
        this.price = price;
    }

    public Book(Book book) {
        this.isbn = book.isbn;
        this.title = book.title;
        this.author = book.author;
        this.price = book.price;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        Book other = (Book) obj;
        if (isbn == null) {
            if (other.isbn != null)
                return false;
        } else if (!isbn.equals(other.isbn))
            return false;
        return true;
    }

    // getters, setters and hashcode omitted
}

BookRepository:

public class BookRepository {
    private Map<String, Book> books;

    public BookRepository() {
        books = new HashMap<String, Book>();
    }

    public List<Book> getAllBooks() {
        return new ArrayList<Book>(books.values());
    }

    public Book getBook(String isbn) {
        if (books.containsKey(isbn))
            return new Book(books.get(isbn));

        return null;
    }

    public boolean addBook(Book book) {
        Book copy = new Book(book);

        if (!books.containsKey(copy.getIsbn()))
            return books.put(copy.getIsbn(), copy) == null;

        return false;
    }

    public boolean deleteBook(String isbn) {
        return books.remove(isbn) != null;
    }

    public boolean updateBook(Book book) {
        Book copy = new Book(book);

        if (books.containsKey(copy.getIsbn()))
            return books.put(copy.getIsbn(), copy) != null;

        return false;
    }
}

Your code is nice and the only advice I can give you is this: getAllBooks() may be improved from linear to constant time.

Consider

public boolean addBook(Book book) {
    Book copy = new Book(book);

    if (!books.containsKey(copy.getIsbn())) {
        this.list.add(book);
        return books.put(copy.getIsbn(), copy) == null;
    }

    return false;
}

public List<Book> getAllBooks() {
    return Collections.unmodifiableList(this.list);
}

Hope that helps.


Improving function that replaces all instances of x with y in string I have the following:

#include <string>
#include <string_view>
#include <vector>
#include <unordered_map>

#include <iostream>

namespace utils
{
    void replace_all(std::string& string, const std::string_view from, const std::string_view to)
    {
        std::size_t start_pos{ 0 };
        while ((start_pos = string.find(from)) != std::string::npos)
        {
            string.replace(start_pos, from.length(), to);
            start_pos += to.length();
        }
    }

    void replace_all(std::string& string, const std::unordered_map<std::string_view, std::string_view>& map) // Warning 2 here
    {
        for (const auto [from, to] : map) // Warning 1 here
        {
            replace_all(string, from, to);
        }
    }
}

int main()
{
    std::unordered_map<std::string_view, std::string_view> my_map{ {"this", "that"}, {"hes", "her"}, {"my", "yours"} };
    std::string my_string{ "This is hes chocolate and my aswell" };
    utils::replace_all(my_string, my_map);
    std::cout << my_string << std::endl;

    return 0;
}

It works and outputs: This is her chocolate and yours aswell as expected.

Analysing this in MSVC 2019 with C++ Core guidelines turned on I do get the following warnings though:

C:\dev\MyVSProjects\AE\AE\include\AE\Main.cpp(23): warning C26486: Don't pass a pointer that may be invalid to a function. Parameter 0 '$S1' in call to '<move><std::pair<std::basic_string_view<char,std::char_traits<char> > const ,std::basic_string_view<char,std::char_traits<char> > > const & __ptr64>' may be invalid (lifetime.3).

C:\dev\MyVSProjects\AE\AE\include\AE\Main.cpp(21): warning C26487: Don't return a pointer '(*(*map))' that may be invalid (lifetime.4).

I'd like to remove these warnings and make the code as clean as possible.

This code is nicely written and clear in both code and description.  Good job!  I think there are still some things that might be improved.

Fix the bug

The updated value of start_pos is not being used for each iteration of the loop.  Instead, it should be, so the while loop should be this:

while ((start_pos = string.find(from, start_pos)) != std::string::npos)

Avoid creating copies if practical

I don't have MSVC 2019 available, but I suspect that it's pointing out that you've asked it to make copies of from and to.  That is, instead of this:

for (const auto [from, to] : map)

Write this:

for (const auto& [from, to] : map)

And see if that helps.  Another alternative would be to create a version of replace_all that takes a std::pair<std::string_view, std::string_view> and rewrite the loop as:

for (const auto& pr: map) {
    replace_all(string, pr);
}

Consider using a different data structure

The std::unordered_map used here may seem like a logical and intuitive structure to use but it has some potential problems.  First, the iterator has no defined ordering.  That is, the ordering is implementation defined so the programmer has no control of it.  That might be a problem if we want to do things in a specific order.  For example, replacing literal characters with their corresponding predefined XML entities, such as > to &gt; we typically want to do the & to &amp; conversion first for obvious reasons.  There's no way to do that with the existing version of the code.  Second, we can't create a constexpr version of the map because it has a non-trivial destructor.  I'd suggest allowing something like a std::array of std::pair<std::string_view, std::string_view>.  This is the approach that is used in conjunction with the next few suggestions.

Use using to simplify code

The existing code could be simplified a bit by using using like so:

using MapContainer = std::unorded_map<std::string_view, std::string_view>;
void replace_all(std::string& string, const MapContainer& map)  { /* etc. */ }

Now any place we use the rather long name we can simply write MapContainer.

Use templates for flexibility

Instead of tying the replace_all function to a specific kind of structure, we could pass it a pair of templated iterators instead for flexibility:

template <class ForwardIt>
void replace_all(std::string& string, ForwardIt first, ForwardIt last) {
    for ( ; first != last; ++first) {
        replace_all(string, *first);
    }
}

Now all we need is any forward iterator that returns something that can be used by the inner replace_all.  In fact, we can simplify even further because this is exactly like the definition of std::for_each.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Consider creating a parallel version

The current version is simple and straightforward, but makes multiple passes through the string.  It's possible to create a version that makes a single pass through or that makes multiple passes in parallel.  The single pass version could be general purpose, but the parallel version would require that patterns be completely independent (unlike the entity-replacing example I mentioned).

Consider eliminating return 0

You don't need to explicitly provide a return 0; at the end of main -- it's created implicitly by the compiler.  Some people apparently feel very strongly both for and against this.  I advocate omitting it but you can decide for yourself.

Alternate version

Here's what I came up with using all of these suggestions:

#include <string>
#include <string_view>
#include <array>
#include <algorithm>
#include <iostream>

namespace utils
{
    using WordPair = std::pair<std::string_view, std::string_view>;
    void replace_all(std::string& string, const WordPair &pr) {
        for (std::size_t start_pos{ 0 };
            (start_pos = string.find(pr.first, start_pos)) != std::string::npos;
            start_pos += pr.second.length())
        {
            string.replace(start_pos, pr.first.length(), pr.second);
        }
    }
}

int main()
{
    constexpr std::array<utils::WordPair, 5> my_map{{
        {"&", "&amp;"},
        {"<", "&lt;"},
        {">", "&gt;"},
        {"'", "&apos;"},
        {"\"", "&quot;"}
    }};
    std::string my_string{ "This & that aren't \"<>\"." };
    // use a lambda for clarity
    auto repl = [&my_string](const utils::WordPair &p){
            utils::replace_all(my_string, p);
    };
    std::for_each(my_map.cbegin(), my_map.cend(), repl);
    std::cout << my_string << '\n';
}


Use of C# Razor Syntax in MVC.net view I have the code below and I am looking for input as to how I can improve it.

The main question here is should the code at the top in razor brackets be moved to the Controller and returned via a ViewModel?

The underlying question being, particularly when the code isnt controling styling is should the endeavour be to have as much as the C# code inside the controller as much as possilble, which i feel i am good at doing, and getting better at despite a few areas of lazyness.

@{
    var show = ViewBag.ShowBasket == "true";

    var visiblityLg = show ? "visible-lg" : "hidden hidden-lg";
    var visiblityMd = show ? "visible-md" : "hidden hidden-md";
    var visiblitySm = show ? "visible-sm" : "hidden hidden-sm";
    var visiblityXs = show ? "visible-xs" : "hidden hidden-xs";
}

    &nbsp;Shopping Basket

I would argue that the code in the top should not be moved to Controller.

This of course depends on how you define responsibilities for View, Controller and Model, but I would leave the code in the View because Controller and Model should not have knowledge about CSS if it can be avoided.

More about responsibilities in ASP.NET MVC