Fastest way of removing a substring from a string Taking a break from my C++ series, I recently reviewed the code in this question.  I ended up writing my own separate derivation of the function so that I could remove all of the substrings from a certain string.

#include <stdio.h>
#include <string.h>
#include <time.h>

#define NUM_CYCLES 10000000

void rmSubstr(char *str, const char *toRemove)
{
    size_t length = strlen(toRemove);
    while((str = strstr(str, toRemove)))
    {
        memmove(str, str + length, 1 + strlen(str + length));
    }
}

clock_t runTest(void (*fn)(char*, const char*), char *str, char *substr)
{
    clock_t start = clock();
    for (int i = 0; i < NUM_CYCLES; ++i)
    {
        fn(str, substr);
    }
    return clock() - start;
}

int main(void)
{
    char* str1 = strdup("test this string out");
    char* str2 = strdup("test this ...........slightly............ longer thing which contains a sub string out");
    char* str3 = strdup("string");
    char* str4 = strdup("this string is not a string test");

    printf("Function ran in %lu clock cycles\nProduced output: %s\n\n", runTest(rmSubstr, str1, " string"), str1);
    printf("Function ran in %lu clock cycles\nProduced output: %s\n\n", runTest(rmSubstr, str2, " string"), str2);
    printf("Function ran in %lu clock cycles\nProduced output: %s\n\n", runTest(rmSubstr, str3, " string"), str3);
    printf("Function ran in %lu clock cycles\nProduced output: %s\n\n", runTest(rmSubstr, str4, " string"), str4);
}

Generates on my MacBook Pro (Intel Core i5 2.4 GHz with 2 cores, 4 GB RAM, running Mac OS X 10.10 beta 4):

Function ran in 319220 clock cycles
Produced output: test this out

Function ran in 1293377 clock cycles
Produced output: test this ...........slightly............ longer thing which contains a sub out

Function ran in 198460 clock cycles
Produced output: string

Function ran in 448832 clock cycles
Produced output: this is not a test

Program ended with exit code: 0

The only thing I would like to be reviewed in this question is the speed of the rmSubstr function.  Other comments are welcome, but alone will not be accepted.  Also, please include the output of the program with your answer so that I can get a feeling for how much my function was improved.

It should be possible to optimize a global search-and-replace by doing all the moves in one pass.

void rmSubstr(char *str, const char *toRemove)
{
    size_t length = strlen(toRemove);
    char *found,
         *next = strstr(str, toRemove);

    for (size_t bytesRemoved = 0; (found = next); bytesRemoved += length)
    {
        char *rest = found + length;
        next = strstr(rest, toRemove);
        memmove(found - bytesRemoved,
                rest,
                next ? next - rest: strlen(rest) + 1);
    }
}

Output using your test code:

Function ran in 351851 clock cycles
Produced output: test this out

Function ran in 1349681 clock cycles
Produced output: test this ...........slightly............ longer thing which contains a sub out

Function ran in 197570 clock cycles
Produced output: string

Function ran in 416682 clock cycles
Produced output: this is not a test

Program ended with exit code: 0


Which linq statement is better to find there is an overlap between two lists of ints? I'm wondering about the difference between these two linq statements

bool overlap = GetMyListA().Intersect(GetMyListB()).Any(); // 1.

vs

bool overlap = GetMyListA().Any(i => GetMyListB().Contains(i)); // 2.

Will statement 2. call GetMyListB() for each item in ListA?

Which is more readable?

Assuming LINQ to objects (i. e. these are in-memory collections not LINQ to Entities IQueryables or something):

  Will statement 2. call GetMyListB() for each item in ListA?

Yes. If you want to avoid this, you'll have to store the result of GetMyListB() outside the function.

  Which is more readable?

In my opinion two are about equally readable, although I would change #2 to be:

bool overlap = GetMyListA().Any(GetMyListB().Contains); // 2.

As far as performance, #1 will probably perform better in most cases, since it will dump all of one list into a hash table and then do an O(1) lookup for each element in the other list. Two exceptions to this that I can think of are (1) if the lists are very small, then building the hash table data structure might not be worth the overhead and (2) option 2 can theoretically return without having considered every element in either list. If the lists are large lazy enumerables and you expect this to return true in most cases then this could lead to a performance increase.


Combining two lists into one Can you improve?

rc = []
for i, j in zip(A_response, B_response):
    rc.append(i)
    rc.append(j)

I use zip and append, but could this be better using list comprehension?

A good solution is in this Stack Overflow answer:

rc = [response for ab in zip(A_response, B_response) for response in ab]


Select data from csv and make associative array I've got a .csv file with ZIP codes containing the city and its sub-districts

67401,City 1,district_a
67401,City 2,district_b
67401,City 3,district_c
67401,City 3,district_d
67401,City 3,district_e
67401,City 3,district_f
67401,City 3,district_g
67401,City 3,district_h
67401,City 3,district_i
67401,City 3,district_j
67401,City 3,district_k
67401,City 3,district_l
67401,City 3,district_m
67401,City 3,district_n
67401,City 3,district_o
67401,City 3,district_p
67401,City 3,district_q
67401,City 3,district_r
67401,City 3,district_s
67401,City 4,district_t
67401,City 5,district_u

67501,City 6,district_v
67501,City 7,district_w
67501,City 8,district_x
67501,City 8,district_y
67501,City 8,district_z

And I need to put it in an array, where I can select/search by the zip code (67401, 67501, etc.) and return these results as an associative array in this format, so I can populate a select control.

For example I want to look-up all the Cities and its districts with zip code 67401 I need this result:

"City 1" => [
    district_a => district_a
],
"City 2" => [
    district_b => district_b
],
"City 3" => [
    district_c => district_c
    district_d => district_d
    district_e => district_e
    district_f => district_f
    district_g => district_g
    district_h => district_h
    district_i => district_i
    district_j => district_j
    district_k => district_k
    district_l => district_l
    district_m => district_m
    district_n => district_n
    district_o => district_o
    district_p => district_p
    district_q => district_q
    district_r => district_r
    district_s => district_s
],
"City 4" => [
    district_t => district_t
],
"City 5" = [
    district_u => district_u
]

Right now I've got this working code, but I'm sure there has to be a simpler way to achieve it.

<?php
$zipcode = '67401';

$a = file("zip.csv", FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
$a = array_map(function ($i) {return explode(',', $i); }, $a);
$a = array_filter($a, function ($i) use ($zipcode) { return $i[0] == $zipcode; });

$b = [];
$i = 0;
foreach ($a as $key => $value) {
    $b[$i++] = [
        'city' => $value[1],
        'district'  => $value[2],
    ];
}

$c = [];

foreach($b as $key => $item) {
   $c[$item['city']][$key] = $item;
}

ksort($c, SORT_REGULAR);

$d = [];
foreach($c as $key => $value) {
    $subarray = [];
    foreach($value as $s_key => $s_val) {
        foreach($s_val as $ss_key => $ss_val) {
            $subarray[$ss_val] = $ss_val;
        }
    }
    $d[$key] = $subarray;
}

$myresult = $d;
?>

Well, first of all this data must be stored in a database, not a file. Using PDO you'll get your array in a few lines (assuming a database connection is already established):

$sql = "SELECT city, district FROM zip WHERE zipcode=? ORDER BY city, district";
$stmt = $pdo->prepare($sql);
$stmt->execute([$zipcode]);
$data = $stmt->fetchAll(PDO::FETCH_GROUP|PDO::FETCH_COLUMN);

As of the code present, there are way too much loops to my taste. I believe everything could be done in one loop, like

$zipcode = '67401';
$data = [];
foreach (file("zip.csv", FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES) as $row) {
    list($zip, $city, $district) = explode(",",$row);
    if ($zip == $zipcode) {
        if (!isset($data[$city])) {
            $data[$city] = [];
        }
        $data[$city][] = $district;
    }
}

well if you need to sort your arrays, a couple extra loops are still needed

ksort($data);
foreach ($data as $city => $array) {
    sort($data[$city]);
}


Vue Star Rating Component I am wondering if someone can do a code re"vue" specifically for my star rating component ([complete code available on github).

I am looking for feedback on best practices, any glaring code crimes I might have committed and bad code.

The code is also deployed to #Heroku here

Problems I saw with the code previous code are also included.

The partial star sometimes just gets the default colors, even if the DOM has the colors assigned via props.
Change in data (using Vue Dev Tools) does not re-render my star rating component. For the benefit for the code reviewers, I have hosted the component on Heroku, so if you have the Vue dev tools, you can test this
too.

Edit - Inlined the code to follow the code-review standards.

Edit - 10/23 - I have updated the code since I first posted this and seem to have resolved all the issues with my previous code. The link to Github is here. Also the inlined code is now newer... I have added comments in the code, where I would love some feedback and comments.

    <template>
    <div>
        <!-- Is this use of dynamic component correct? -->
        <component v-for="n of this.totalStarsData" :key="n" :is="currentComponent(n)" :fillColor="ratingData - n > -1 ? fillColorData : baseColorData" :baseColor="baseColorData" :rating="ratingData"></component>
    </div>
    </template>

    <script>
// Can Star and PartialStar components be dynamically imported?
import Star from "./Star"
import PartialStar from "./PartialStar"

// Using a global variable like this - Good or bad?
let isPartialRendered = false
export default {
    name: 'star-rating',
    components: {
        Star,
        PartialStar
    },
    props: {
        totalStars: {
            type: Number,
            default: 5
        },
        rating: {
            type: Number,
            required: true
        },
        fillColor: {
            type: String,
            default: '#C00'
        },
        baseColor: {
            type: String,
            default: '#666'
        },
    },
    data(){
        return {
            // Any better way of converting props to data?
            // Also maybe naming convention best practices?
            totalStarsData: this.totalStars,
            ratingData: this.rating,
            fillColorData: this.fillColor,
            baseColorData: this.baseColor,
        }
    },
    methods: {
        currentComponent: function (count) {
            const int_part = Math.trunc(this.ratingData);
            if(count > int_part && isPartialRendered === false) {
                isPartialRendered = true;
                // Re-setting the isPartialRendered flag here, for cases where the Partial SVG is the last SVG
                if(count === this.totalStarsData){
                    isPartialRendered = false;
                }
                return 'PartialStar'
            } else {
                if(count === this.totalStarsData){
                    isPartialRendered = false;
                }
                return 'Star'
            }

        }
    }
}
</script>

<style scoped>

</style>

Converting props to data is a bad idea if it is not a copy of the prop because you might end up in a situation where your parent changes your prop and your component changes your data and then which is right? The component manipulating props is not good. Instead decide which is externally sourced (from parent) and which is internally manipulated, and then assign each to either prop or data.

Since your code in particular doesn't actually manipulate the data or props you are simply better off if you remove the data key completely, just leave it with props and you can access the values in precisely the same way using this.totalStarsData for example. You can clearly see that having a props key and manually entering the same info into data is ugly code anyway.

Having that global variable is unhelpful in my opinion, and probably bad practice since it goes against the decoupling of the component. Why not make it another prop with a default value of false?


Notifying Students in Courses - Strategically I have the following implementation, and hope to get suggestions on how to improve the code.

treeListViewModel.Courses is an observablecollection
treeListViewModel.Courses[i].Students is a dictionary

The code loops as follows:

        List<uint> DataNotificationList = new List<uint>()
        {
            2,
            6,
            9
        };

        for (int n = 0; n < DataNotificationList.Count; n++)
        {
            for (int i = 0; i < treeListViewModel.Courses.Count; i++)
            {
                if (treeListViewModel.Courses[i].Students.ContainsKey(DataNotificationList[n]))
                {
                    treeListViewModel.Courses[i].Students[DataNotificationList[n]].ExecuteStrategy(hasError());
                    break;
                }
            }
        }

This smells of a message chain and violates the Law of Demeter (principle of least knowledge):

treeListViewModel.Courses[i].Students[DataNotificationList[n]].ExecuteStrategy(hasError());

Your caller very like doesn't care of the exact internal workings of a course - it should just dispatch the notifications and see if it succeeded. So a Course should probably get a method like bool NotifyStudent(int studentId, bool hasError).
When you use expressions like DataNotificationList[n] in several places then it is usually better to put it in a local variable which expresses the meaning of it and also makes the code shorter, for example like this: int studentId = DataNotificationList[n];
If you don't actually need the index then foreach will usually suffice and is shorter.

So in total your refactored code could look like this:

    foreach (int studentId in DataNotificationList)
    {
        foreach (Course course in treeListViewModel.Courses)
        {
            if (course.NotifyStudent(studentId, hasError()))
            {
                break;
            }
        }
    }

or if you want to go with a bit more LINQ:

    foreach (int studentId in DataNotificationList)
    {
        var course = treeListViewModel.Courses
                        .FirstOrDefault(c => c.NotifyStudent(studentId, hasError()));
        // could log something here and maybe raise an event handler
        // to say that a student in a course has been notified
    }

The NotifyStudent method in the course is responsible to check if the student exists in the course and return true once it has executed the strategy. It is totally encapsulated in the course and the caller doesn't have to know about any internals.


MySQL Prepared Statement - Combinging 3 seperate statements in to 1 statement I'm currently learning MySQL about prepared statements and I was wondering if this code I've written is the most efficient way.
The process:

There's a query that checks a table called 'websites' for a field called websites_pagecount.
The same query then increments the field by 1. (I haven't done this is the database using auto-increment because there will be more than one row with the same number.)

There's a query that then runs and grabs the new number.

Then another query runs and creates a row in a table called pages and sets the new pagecount number in to the page_id field (again, I haven't used auto-increment on this as there will be more than one row with the same number).

However, it all seems a bit 'cumbersome'.
$mysql['stmt'] = $mysql['conn']->prepare("UPDATE websites SET website_pagecount=website_pagecount+? WHERE website_domain=?");
$mysql['stmt']->bind_param("is", $a = 1, $website['website_id']);
$result = $mysql['stmt']->execute();
$mysql['stmt']->close();

$mysql['stmt'] = $mysql['conn']->prepare("SELECT website_pagecount FROM websites WHERE website_domain = ?");
$mysql['stmt']->bind_param("s", $website['website_id']);
$result = $mysql['stmt']->execute();
//GET ROWS
$result = $mysql['stmt']->get_result();
$pagecount = $result->fetch_assoc();
$mysql['stmt']->close();

$mysql['stmt'] = $mysql['conn']->prepare("INSERT INTO pages (page_id,page_parent, page_title, page_description,page_keywords, website_id, page_layout) VALUES (?,?,?,?,?,?,?)");
$mysql['stmt']->bind_param("issssss",$pagecount['website_pagecount'],$item['parentpage'],$item['pagetitle'],$item['pagedescription'],$item['pagekeywords'],$website['website_id'],$item['pagelayout'] );
$result = $mysql['stmt']->execute();
$mysql['stmt']->close();

The code is used to create a new web page from a web based administration panel. The admin panel can be used to edit multiple websites (depending on which user is logged in) All website information is held in the 'websites' table and all the pages from all the websites are held in the 'pages' table.
Every page has an auto-incrementing unique ID, however they also have a page id relative to the website they belong to. IE (test.com can have pages with ids 1,2 and 3 however testagain.com can also have pages with ids 1,2,3)
The reason for the 'website_pagecount' field is so when a new page is created for a particular website, it counts up in order.
The reason I have done this is because if i used a 'row count' function, for example, to generate the page ID field, and pages with the ID's 1 & 3 got deleted, when a new page was generated it would set the ID to '2' - however that would conflict with the already existing page that has an ID set which was 2.
If there was to be 30-40 pages for a website and pages got deleted and created over time, there would be an awful lot of conflicts eventually per website.

Thanks for the comments. I understand what your webpage does now.

Suggestion #1 - Descriptions Of What Your Code Does - You might want to create some functions (or add some comments) just to make it more clear what each segment of code does. The OOP people like to make tons of functions that describe chunks of code, and the non-OOP people like comments. You can go either way, in my opinion. In my example here, I will use functions.

Suggestion #2 - More Whitespace - I would also add some spaces after your commas and maybe some enters after your commas in your bind_param() functions so that everything wraps nicely and is easy to read. You can do the same thing with your prepare() functions. It isn't a big deal for single line SQL statements like "SELECT * FROM table WHERE field = 'value'", but it starts becoming very helpful when you're doing long, complex INNER JOINS and other complicated SQL statements.

<?php

// control panel for multiple websies

$website = define it up here;
$item = define it up here;

function add_page($website_id, $item, $mysql)
{
    $next_page_count = increment_and_return_next_page_count($website_id, $mysql);

    create_new_page($item, $next_page_count, $mysql);
}

function increment_and_return_next_page_count($website_id, $mysql)
{
    $mysql['stmt'] = $mysql['conn']->prepare("
        SELECT website_pagecount
        FROM websites
        WHERE website_domain = ?
    ");
    $mysql['stmt']->bind_param(
        "s",
        $website_id
    );
    $result = $mysql['stmt']->execute();
    $result = $mysql['stmt']->get_result();
    $pagecount = $result->fetch_assoc();
    $mysql['stmt']->close();

    $pagecount++;

    $mysql['stmt'] = $mysql['conn']->prepare("
        UPDATE websites
        SET website_pagecount=?
        WHERE website_domain=?
    ");
    $mysql['stmt']->bind_param(
        "is",
        $pagecount,
        $website_id
    );
    $result = $mysql['stmt']->execute();
    $mysql['stmt']->close();

    return($pagecount);
}

function create_new_page($item, $next_page_count, $mysql)
{
    $mysql['stmt'] = $mysql['conn']->prepare("
        INSERT INTO pages (page_id,page_parent, page_title, page_description,page_keywords, website_id, page_layout)
        VALUES (?,?,?,?,?,?,?)
    ");
    $mysql['stmt']->bind_param(
        "issssss",
        $next_page_count,
        $item['parentpage'],
        $item['pagetitle'],
        $item['pagedescription'],
        $item['pagekeywords'],
        $website['website_id'],
        $item['pagelayout']
    );
    $result = $mysql['stmt']->execute();
    $mysql['stmt']->close();
}

add_page($website['website_id'], $item, $mysql);

?>

Other than those tips, I will defer to more experienced PHP coders. I hope my ideas help you out a little bit though. Good luck!

P.S. As for your original idea of combining the statements: I don't think they can be combined because they are doing different things. One is SELECT, another is UPDATE, and the third is INSERT. If they were all doing the same thing, then you could combine them using various types of JOINS probably. But since they are performing different actions I don't think that is an option.


Handling exceptions from async call in synchronous caller I am using this pattern to easily handle exceptions from async code in a non-async caller. Is this a reasonable thing to do, or am I missing something? In this relatively simply case, I have a function called "SendCommand" that does nothing more than call ProcessCommand, which is an async function. I'm doing this because SendCommand is exported in a Xamarin Android project so that it can be called from my Main.axml layout file (via a reference from onClick), which, as far as I can tell, cannot call async functions.

[Java.Interop.Export("SendCommand")]
public void SendCommand(View view)
{
   try
   {
      ProcessCommand().ContinueWith((t) =>
      {
         try
         {
            t.Wait();
         }
         catch (Exception ex)
         {
            ShowMessage(ex.Message);
         }
      });
   }
   catch(Exception ex)
   {
      ShowMessage(ex.Message);
   }
}

The odd thing about this is that I'm calling wait on a task I already know is complete just to throw any exceptions that may need yet to be handled (converted into proper exceptions in this stack).

You're correct that you don't need Wait to inspect the Exception of a completed Task.  You can use the Exception property to look at the exception.  You can also use the only on faulted continuation option if you only want to perform the action if the task faults.  This lets you turn the continuation into:

ProcessCommand()
    .ContinueWith(t => ShowMessage(t.Exception.Message),
        TaskContinuationOptions.OnlyOnFaulted);

Next, you handle the case where ProcessCommand throws, rather than returning a faulted Task.  If at all possible, I'd avoid the situation entirely.  If it's your own method, redesign it such that it only ever returns a faulted task if there's an exceptional problem, rather than throwing, rather than handling both cases any time you ever want to add a continuation to any asynchronous operation.  If you're calling method you don't control, like an API that's constructed to both throw and return faulted tasks, then yes, you do need to handle both, but this should be rare.  If you're writing code like this often, then you're probably doing something wrong.  If it is something you find yourself doing a lot, then at a minimum write a method that does the boilerplate, rather than repeating it:

public static Task WrapExceptions(Func<Task> asyncFunction)
{
    try
    {
        return asyncFunction();
    }
    catch (Exception e)
    {
        return Task.FromException(e);
    }
}
public static Task<T> WrapExceptions<T>(Func<Task<T>> asyncFunction)
{
    try
    {
        return asyncFunction();
    }
    catch (Exception e)
    {
        return Task.FromException<T>(e);
    }
}

Now you can write WrapExceptions(ProcessCommand) (If ProcessCommand cannot be refactored itself) and have a method that never throws, and only ever returns a faulted task in exceptional situations.


Filtering a TreeView I use the code below for filtering a treeview. Can this be improved?

private IEnumerable<TreeNode> FindNodeByValue(TreeNodeCollection nodes, string searchstring)
 {

    foreach (TreeNode node in nodes)
    {
        if (node.Value.IndexOf(searchstring,
              StringComparison.CurrentCultureIgnoreCase) >= 0)
            yield return node;
        else
        {
            foreach (var subNode in FindNodeByValue(node.ChildNodes, searchstring))
                yield return subNode;
        }
    }
}

protected void Button1_Click(object sender, EventArgs e)
{

    var query= FindNodeByValue(TreeView1.Nodes, fieldFilterTxtBx.Text);
    if (query != null)
    {
        //TreeView1.Nodes[0].Expand();
        //TreeView1.Nodes.Clear();
        foreach (TreeNode node in query.ToList())
        {
            TreeView1.Nodes.Add(node);

        }

        //  TreeNode newnode = new TreeNode("Detail Engineering");
        // TreeView1.Nodes.Add(newnode);

        TreeView1.ExpandAll();
    }

    else
    {

        Label1.Text = "No file found";

    }
}

What's the problem with your implementation? You clear the tree and add new (filtered) nodes. Somehow, TreeView1.Nodes.Clear(); is under comment, so it may be supposed that you want to preserve full node structure. If so, just store it in memory:

var query = FindNodeByValue(PersistentNodeSet.Nodes, fieldFilterTxtBx.Text);
if (query != null)
{
    TreeView1.Nodes.Clear();
    foreach (TreeNode node in query.ToList())
        TreeView1.Nodes.Add(node);
    TreeView1.ExpandAll();
}

And if you want the tree to preserve original structure, you need to use recursion in FindNodeByValue().


Langton's Ant cellular automaton in Ruby I wrote a Langton's Ant cellular automaton simulator in Ruby using object oriented principles. This code works. It requires the drawille library.

I would like to get feedback on what I could do better here, from optimizing the display to cleaning up code. I'm also wondering what I could improve in code quality and style.

#!/usr/bin/env ruby

require 'drawille'

# Langton's Ant
# Draws in terminal

class Direction
    @dir = 0

    def initialize(direction)
        @dir = {up: 0, right: 1, down: 2, left: 3}[direction] || 0;
    end

    def transform_vector
        case @dir
        when 0
            return {x: 0, y: 1}
        when 1
            return {x: 1, y: 0}
        when 2
            return {x: 0, y: -1}
        when 3
            return {x: -1, y: 0}
        end
    end

    def turn_cw
        @dir = (@dir + 1) % 4
    end

    def turn_ccw
        @dir = (@dir + 3) % 4
    end
end

module State
    OFF = 0
    ON = 1
end

class Tile
    @state = State::OFF
    def initialize(state)
        @state = state
    end
    def state
        return @state
    end
    def flip
        if @state == State::OFF
            return @state = State::ON
        else
            return @state = State::OFF
        end
    end
    def bool
        return (@state == State::ON ? true : false)
    end
end

class Screen
    @screen = [];
    @canvas = Drawille::Canvas.new
    @x = 0
    @y = 0

    def initialize(x, y)
        @screen = Array.new(y) { Array.new(x) { Tile.new State::OFF } }
        @canvas = Drawille::Canvas.new
        @x = x
        @y = y
    end

    def inspect
        string = "";
        @screen.each do |i|
            i.each do |j|
                string << (j.bool ? '██' : '  ')
            end
            string << ?\n
        end
        return string
    end

    def screen
        return @screen
    end

    def flip(x, y)
        return @screen[y][x].flip
    end

    def print
        @screen.each_with_index do |row, y|
            row.each_with_index do |cell, x|
                if cell.state == State::ON
                    @canvas.set(x, y)
                else
                    @canvas.unset(x, y)
                end
            end
        end
        puts @canvas.frame
    end

    def x
        @x
    end

    def y
        @y
    end
end

class Ant
    @x = 0
    @y = 0
    @direction = :up
    @screen = nil

    def initialize(x, y, direction, screen)
        @x, @y, @direction, @screen = x, y, direction, screen
    end

    def current_cell
        @screen.screen[@y][@x]
    end

    def step
        if current_cell.state == State::ON
            @direction.turn_cw
        else
            @direction.turn_ccw
        end
        @screen.flip(@x, @y)
        @x = (@x + @direction.transform_vector[:x]) % @screen.x
        @y = (@y + @direction.transform_vector[:y]) % @screen.y
    end
end

def main
    screen = Screen.new(160, 96);
    ant = Ant.new(79, 47, Direction.new(:down), screen);

    loop do
        screen.print
        ant.step
    end
end

main

The code looks pretty clean. On first sight I notice some odd Ruby code.

Non-idiomatic Ruby

class Tile
    @state = State::OFF
    def initialize(state)
        @state = state
    end
    def state
        return @state
    end
    def flip
        if @state == State::OFF
            return @state = State::ON
        else
            return @state = State::OFF
        end
    end
    def bool
        return (@state == State::ON ? true : false)
    end
end

In the code above you first set @state and then inside initialize you set @state again. These two are not the same variable. Do not define instance variables outside class methods, unless you want to do some meta-programming with them.

You define method state for accessing the @state variable. As this is such a common pattern, Ruby provides a helper for generating such simpler getter methods:

class Tile
    attr_accessor :state

But all the places where state method is used, could actually use the bool method instead. This should make one realize that this whole Tile class could be completely eliminated and replaced with a plain boolean value instead.

Other smells

Screen class has @screen instance variable. It's not referring to itself, so a better name would be in order.
It's also exposing it directly to the outside world through screen method. Better to have a method like get(x,y) that would encapsulate the access to the coordinates array.
Screen has an inspect method that's unused. For debugging?
Screen has x and y fields, I think these are better called width and height.
Screen has a print method. I would instead recommend having a to_s method instead, so the Screen would not have a knowledge about printing itself, only of how to convert itself into string. One can then simply call puts screen to print it to console (the to_s method will be call automatically when casting to string).
Inside Ant.initialize all the variables are initialize on the same line, which I find confusing.


Determine if a String has a mirroring head and tail Problem statement:

  Given a string, look for a mirror image (backwards) string at both the
  beginning and end of the given string. In other words, zero or more
  characters at the very begining of the given string, and at the very
  end of the string in reverse order (possibly overlapping). For
  example, the string "abXYZba" has the mirror end "ab".

Examples:

mirrorEnds("abXYZba") → "ab"
mirrorEnds("abca") → "a"
mirrorEnds("aba") → "aba"

Below is my solution to the problem in java:

public String mirrorEnds(String string) {
    final int len = string.length();
    final int half = len / 2;
    String result = "";

    for (int i = 0; i < half; i++) {
        if (string.charAt(i) != string.charAt(len -1 -i)) {
            break;
        } else {
            result += string.substring(i, i + 1);
        }
    }
    return result.length() == half ? string : result;
}

Is it safe to say that in terms of time complexity the solution is optimal already? Any other review comments are also welcome.

Below your question:

  Is it safe to say that in terms of time complexity the solution is
  optimal already?

Yes, you are comparing chars from front and back of the string and stop when you encounter two different chars so this is a complexity O(n).

Some minor changes to your code, instead of iterate over your string transform it to a char array and instead of break the cycle return directly the result with the use of a StringBuilder for the result:

char[] arr = string.toCharArray();

StringBuilder builder = new StringBuilder();
for (int i = 0; i < half; ++i) {
    if (arr[i] != arr[len -1 -i]) {
        return builder.toString();
    }
    builder.append(arr[i]);
}

return string;

In this way you avoid the use of consecutive creation of substrings and the code is simpler.

Your method can be rewritten then in this equivalent way:

public static String mirrorEnds(String string) {
    final int len = string.length();
    final int half = len / 2;
    char[] arr = string.toCharArray();

    StringBuilder builder = new StringBuilder();
    for (int i = 0; i < half; ++i) {
        if (arr[i] != arr[len -1 -i]) {
            return builder.toString();
        }
        builder.append(arr[i]);
    }

    return string;
}


Count the occurrence of each unique word in the file I've been doing a task for an interview that I will have soon.
The requirement is to code the following problem in C#.

  Write a program (and prove that it works) that: Given a text file, count the occurrence of each unique word in the file.
  For example; a file containing the string “Go do that thing that you do so well” should find these counts:
  1: Go
  2: do
  2: that
  1: thing
  1: you
  1: so
  1: well

I coded my solution and it's working fine for the tests I gave.

I'm looking for any ways that I can improve my code and to get feedback on the solution. Is there's any way I could to the problem more efficiently.

Thank you in advance.

using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;

namespace CountOccurrence
{
    class Program
    {
        static void Main(string[] args)
        {
            string text = System.IO.File.ReadAllText(@"F:\Ex\Myfile.txt");              // User ReadAllText to copy the file's text into a string
            string textToLower = text.ToLower();                                        // Converts the string to lower case string
            Regex reg_exp = new Regex("[^a-z0-9]");                                     // Regular expressions to replace non-letter and non-number characters with spaces. It uses the pattern [^a-z0-9].The a-z0-9 part means any lowercase letter or a digit.
            textToLower = reg_exp.Replace(textToLower, " ");                            // The code uses the Regex object’s Replace method to replace the characters that match the pattern with a space character.
            string[] Value = textToLower.Split(new char[] {' '},
            StringSplitOptions.RemoveEmptyEntries);                                     // Split the string and remove the empty entries

            Dictionary<string, int> CountTheOccurrences = new Dictionary<string, int>(); // Create a dictionary to keep track of each occurrence of the words in the string
            for (int i = 0; i < Value.Length; i++)                                       // Loop the splited string
            {
                if (CountTheOccurrences.ContainsKey(Value[i]))                           // Check if word is already in dictionary update the count
                {
                    int value = CountTheOccurrences[Value[i]];
                    CountTheOccurrences[Value[i]] = value + 1;
                }
                else                                                                     // If we found the same word we just increase the count in the dictionary
                {
                    CountTheOccurrences.Add(Value[i], 1);
                }
            }

            Console.WriteLine("The number of counts for each words are:");
            foreach (KeyValuePair<string, int> kvp in CountTheOccurrences)
            {
                Console.WriteLine("Counts: " + kvp.Value + " for " + kvp.Key);           // Print the number of counts for each word
            }
            Console.ReadKey();
        }
    }
}

Efficiency

I won't say much about efficiency - because without a clear use-case it will be hard to know whether possible changes would be worth the effort - but my main concern would be fact that you force a whole file into a string, of which you immediately produce a second copy. It would be nice to see a version which takes a stream of some description rather than a whole file, as this could (in theory) cope with very large files (ones which can't fit in memory), could have much better memory characteristics, and with a bit of effort could start processing the file before it has read the whole thing, so that you are not stalled waiting for the whole file before you can begin (though an asynchronous implementation would be necessary to fully exploit such possibilities).

API

Don't hard-code the input file-name. Put this code in a nicely packaged method, and take the input as a parameter. This parameter could be a file-name, but it could also be a Stream or String (if you intend to always read the whole thing at the start) or whatever; you can always provide a convenience method to cover important use-cases.

And don't print the output to the console: if the calling code wants to print the counts to the console, let it do that, but give it the information it needs to do what it wants, rather than deciding what to do with the information for it. Returning the dictionary (perhaps as an IDictionary, so that you aren't coupled to the particular class) produces a much more useful interface.

IDictionary<string, int> CountWordOccurances(string text);

If your specification says that you must be printing these counds to the console, then you can write a method to print out the dictionary (perhaps to an arbitrary TextWriter, rather than only Console.WriteLine, which is no fun to test against), and write another method which composes the two.

Comments

Comments should be useful, explaining why code is doing what it is doing or providing some important context. // Converts the string to lower case string says nothing which text.Tolower() doesn't already, and is liable to rot as the method is modified. The code uses the Regex object’s Replace method to replace the characters that match the pattern with a space character. is far too wordy, and just states what the code is doing, without any indication as to why it might be doing that. We can see that it uses a Regex object, and we can see that that is uses the replace method, and we can see that it replaces matches with a " ": none of this needs clarifying.

Variables

I'm not terribly fond of your variable naming. They don't escape the method, so it doesn't really matter what style you use (though everyone uses lowerCamelCase, see the Microsoft Naming Guidelines), but you must be consistent. Value -> value (or values, since it is a collection). CountTheOccurances is not a great name; counts or wordCounts would scan much better. reg_exp encodes no information beyond that which is clear from the type. Something like letterFilter might be better.

I'd be inclined to ditch textToLower, and just replace text: it's so easy to use the old variable accidentally when they have such similar names. If you wanted the separation to be clear, you could put the reading of the text and to-lowering in a different scope (or even method), so that only textToLower appears for the rest of the method; however, you've already confused matters by re-using textToLower as textToLowerAfterRegex.

Splitting

Your regex bit doesn't really make sense; you are replacing every character that doesn't map to a lower-case latin letter or arabic numeral with a space: where is the specification which tells you what counts as a word or not? I'll leave someone who knows more about unicode to comment on how you should do this sort of thing properly, but your code is deficient, not least because it will cut "naïve" into "na" and "ve".

You can use Regex.Split instead of performing a replacement and then splitting, and use something like \p{L} to cover a wider variety of letters (just for example). A LINQ where can then be used to filter out empty entries.

Regex nonLetters = new Regex(@"[^\p{L}]");
return nonLetters.Split(text).Where(s => s.Length > 0);

A more efficient alternative might be to use a regex which matches words, and return captures instead of splitting; however, again, if performance is a concern, then you need to benchmark under realistic conditions.

Regex wordMatcher = new Regex(@"\p{L}+");
return wordMatcher.Matches(text).Select(c => c.Value);

I'd be strongly inclined to separate this 'extracting words' bit from the 'counting words' bit, and to avoid ToLowering at this point. Indeed, rather than using ToLower() to group words, consider supplying the dictionary a case-insensitive string comparer such as StringComparer.CurrentCultureIgnoreCase.

Counting

Though there is merit in using a separate ContainsKey call and [key] lookup, it's more efficient and a bit tidier to use TryGetValue. Here are 2 obvious ways of using it.

if (CountTheOccurrences.TryGetValue(value[i], out int count))
{
    CountTheOccurrences[value[i]] = count + 1;
}
else
{
    CountTheOccurrences.Add(value[i], 1);
}

// or

int count;
if (!CountTheOccurrences.TryGetValue(value[i], out count))
{
    Count = 0;
}
CountTheOccurrences[value[i]] = count + 1;

Personally I prefer the second one (count doesn't leak if not-found), but the first is closer to what you already have.

Another option is to ditch the loop completely, and use a LINQ GroupBy call, reducing the code complexity alot. In the code below, I write a custom general-purpose CountOccurances method, which could be reused for other purposes, and makes the intention of the code plain without compromising on performance (GroupBy would introduce significant overheads).

Example Rewrite

The below incorporates most of the ideas above, with a couple of other adjustments. The separation of concerns is maybe a little excessive, but while the number of lines of code seems to have increased, essentially all the complexity is hidden away in the completely generic (and potentially widely reusable (I wish LINQ had it already)) CountOccurrences method; the other methods are trivial, but none-the-less encapsulate domain information behind a nice API.

/// <summary>
/// Convenience method which prints the number of occurrences of each word in the given file
/// </summary>
public static void PrintWordCountsInFile(string fileName)
{
    var text = System.IO.File.ReadAllText(fileName);
    var words = SplitWords(text);
    var counts = CountWordOccurrences(words);
    WriteWordCounts(counts, System.Console.Out);
}

/// <summary>
/// Splits the given text into individual words, stripping punctuation
/// A word is defined by the regex @"\p{L}+"
/// </summary>
public static IEnumerable<string> SplitWords(string text)
{
    Regex wordMatcher = new Regex(@"\p{L}+");
    return wordMatcher.Matches(text).Select(c => c.Value);
}

/// <summary>
/// Counts the number of occurrences of each word in the given enumerable
/// </summary>
public static IDictionary<string, int> CountWordOccurrences(IEnumerable<string> words)
{
    return CountOccurrences(words, StringComparer.CurrentCultureIgnoreCase);
}

/// <summary>
/// Prints word-counts to the given TextWriter
/// </summary>
public static void WriteWordCounts(IDictionary<string, int> counts, TextWriter writer)
{
    writer.WriteLine("The number of counts for each words are:");
    foreach (KeyValuePair<string, int> kvp in counts)
    {
        writer.WriteLine("Counts: " + kvp.Value + " for " + kvp.Key.ToLower()); // print word in lower-case for consistency
    }
}

/// <summary>
/// Counts the number of occurrences of each distinct item
/// </summary>
public static IDictionary<T, int> CountOccurrences<T>(IEnumerable<T> items, IEqualityComparer<T> comparer)
{
    var counts = new Dictionary<T, int>(comparer);

    foreach (T t in items)
    {
        int count;
        if (!counts.TryGetValue(t, out count))
        {
            count = 0;
        }
        counts[t] = count + 1;
    }

    return counts;
}

Note that every method has inline-documentation, which describes its job (though I'll grant the summaries are not very good; with a proper spec be able to writer better APIs with better documentation).


Aggregate array values into ranges In five minutes I made a pretty ugly looking function. Can you help before I have to commit the code into history?

  Requirements:

  I would like a function that takes an array of numbers, and returns an
  array of ranges. Ideally remove the duplicate line.

  Sample Input:

Array(1,2,3,4,5,6,10,11,12,13,20,24)

  Output:

Array
(
    [0] => 1-6
    [1] => 10-13
    [2] => 20
    [3] => 24
)

  Winning Entries:

Pseudo code
PHP

My attempt:

$myArray = array(1,2,3,4,5,6,10,11,12,13,20,24);

$rangeArray = array();
$start = $end = current($myArray);
foreach($myArray as $range){
    if($range - $end > 1){
        $rangeArray[] = ($start == $end)?$start:$start."-".$end;
        $start = $range;
    }

    $end = $range;
}
$rangeArray[] = ($start == $end)?$start:$start."-".$end;

print_r($rangeArray);

While these may not be any shorter or more efficient than existing answers, perhaps with a slightly different approach they still bring something to the table and might be considered a little less 'ugly'.

Essentially the GetRanges function I've written does the following:

Take a parameter-supplied array of numbers ($aNumbers), remove any duplicates from the array and then sort them numerically lowest to highest.
We next create a multi-dimensional array ($aGroups) and working sequentially through the array of numbers ($aNumbers): Except for the first number which we just add to $aGroups to get us started, for each consecutive number $i > 0, if the previous number $aNumbers[$i-1] is equal to the current number minus 1 $aNumbers[$i] - 1 then the current number is added to the same group because its consecutive and belongs to that range, otherwise the current number is added to $aGroups as a new sub-array with one entry array( $aNumbers[$i] ).

For options 2 and 3 where the function is expanded upon for a simple array:

A new array ($aRanges) is created and for each $aGroups record, if the first entry of the sub-array is the only entry then a new entry is added to $aRanges for just this number, otherwise a new entry is added to $aRanges specifying the first number in the range followed by a hyphen followed by the last number in the range.

For option 3 where the function is expanded upon to return a string:

The implode function is used to create a comma-separated list of all the entries in the $aRanges array and return this as a string.

Option 1: If you want your function to return a multi-dimensional array:

<?php
function GetRanges( $aNumbers ) {
  $aNumbers = array_unique( $aNumbers );
  sort( $aNumbers );
  $aGroups = array();
  for( $i = 0; $i < count( $aNumbers ); $i++ ) {
    if( $i > 0 && ( $aNumbers[$i-1] == $aNumbers[$i] - 1 ))
      array_push( $aGroups[count($aGroups)-1], $aNumbers[$i] );
    else
      array_push( $aGroups, array( $aNumbers[$i] ));
  }
  return $aGroups;
}

$aNumbers = array( 1, 2, 3, 4, 5, 6, 10, 11, 12, 13, 20, 24 );
print_r( GetRanges( $aNumbers ));

  Output:

Array (
  [0] => Array ( [0] => 1, [1] => 2, [2] => 3, [3] => 4, [4] => 5, [5] => 6 )
  [1] => Array ( [0] => 10, [1] => 11, [2] => 12, [3] => 13 )
  [2] => Array ( [0] => 20 )
  [3] => Array ( [0] => 24 )
)

Option 2: If you want your function to return a simple array:

<?php
function GetRanges( $aNumbers ) {
  $aNumbers = array_unique( $aNumbers );
  sort( $aNumbers );
  $aGroups = array();
  for( $i = 0; $i < count( $aNumbers ); $i++ ) {
    if( $i > 0 && ( $aNumbers[$i-1] == $aNumbers[$i] - 1 ))
      array_push( $aGroups[count($aGroups)-1], $aNumbers[$i] );
    else
      array_push( $aGroups, array( $aNumbers[$i] ));
  }
  $aRanges = array();
  foreach( $aGroups as $aGroup ) {
    if( count( $aGroup ) == 1 )
      $aRanges[] = $aGroup[0];
    else
      $aRanges[] = $aGroup[0] . '-' . $aGroup[count($aGroup)-1];
  }
  return $aRanges;
}

$aNumbers = array( 1, 2, 3, 4, 5, 6, 10, 11, 12, 13, 20, 24 );
print_r( GetRanges( $aNumbers ));

  Output: Array ( [0] => 1-6, [1] => 10-13, [2] => 20, [3] => 24 )

Option 3: If you want your function to return a string:

<?php
function GetRanges( $aNumbers ) {
  $aNumbers = array_unique( $aNumbers );
  sort( $aNumbers );
  $aGroups = array();
  for( $i = 0; $i < count( $aNumbers ); $i++ ) {
    if( $i > 0 && ( $aNumbers[$i-1] == $aNumbers[$i] - 1 ))
      array_push( $aGroups[count($aGroups)-1], $aNumbers[$i] );
    else
      array_push( $aGroups, array( $aNumbers[$i] ));
  }
  $aRanges = array();
  foreach( $aGroups as $aGroup ) {
    if( count( $aGroup ) == 1 )
      $aRanges[] = $aGroup[0];
    else
      $aRanges[] = $aGroup[0] . '-' . $aGroup[count($aGroup)-1];
  }
  return implode( ',', $aRanges );
}

$aNumbers = array( 1, 2, 3, 4, 5, 6, 10, 11, 12, 13, 20, 24 );
echo( GetRanges( $aNumbers ));

  Output:
  1-6,10-13,20,24


Piping operator in c++ I wrote a simple piping operator in c++. I just wanted to make sure that my code was robust, modern c++ code, and made correct use of perfect forwarding.
Here's the code:
#include <concepts>
#include <type_traits>
#include <utility>
#include <vector>
#include <iostream>
#include <deque>

//Accept an object on the left hand side and a function on the right
template <typename Any, typename... Args>
inline auto operator| (const Any& obj, std::invocable<Any> auto func){
    // Correct use of perfect forwarding??
    return func(std::forward<const Any&>(obj));
}

//Version for a functor which accepts a reference. Do I need to cover other cases?
template <typename Any, typename... Args>
inline auto operator| (Any& obj, std::invocable<Any&> auto func){
    return func(std::forward<Any&>(obj));
}

And here is how the user uses it:
int main() {
    auto x = std::vector{1 , 2 , 3 , 4 , 5 , 6};
    // Piping example 0
    auto y = 5 | [] (int x) {return x * 2;}
               | [] (int x) {return x * 3;}
               ;

    std::cout << y << '\n';
    // Piping example 1
    x | [] (std::vector<int>& vec) -> std::vector<int>& {vec.push_back(7); return vec;};

    std::cout << x[6];
    return 0;
}

And if you want to clean up the lambda syntax (I don't mind personally) and have a cleaner, neater syntax you can do:
namespace piping {
    template <typename T>
    auto push_back(const T& obj) {
        return [obj] (auto& vec) -> auto& {vec.push_back(obj); return vec;};;
    }
}

/* ... Some code */

int main() {
    auto x = std::vector{1 , 2 , 3 ,4 ,5 , 6};
    // A bit cleaner...
    x | piping::push_back(7);
}

Did I make correct use of modern c++ and perfect forwarding?
Thanks in advance.

That seems a worthwhile objective.  The syntax looks a bit clunky when we have to write lambdas inline like that, but you'll probably develop a library of useful filters (like standard Ranges ones) that look much more natural.
I don't think we've got the use of forwarding references quite right - and you'll be pleased to hear that fixing that should simplify the code a bit.
What we should do is use typename Any&&.  Although that looks like an rvalue-reference, it's actually a forwarding reference because Any is a template parameter.
Any&& allows the template parameter to bind to either lvalue or rvalue, so we just need the one function:
template <typename Any>
auto operator| (Any&& obj, std::invocable<Any> auto&& func) {
    return func(std::forward<Any>(obj));
}

(Note also the auto&& for func, so it can be a mutable functor if we want).

Having written that, I'm not so convinced that I like operator|() to be modifying its argument.  I would prefer a | b to leave a alone, and have to write a |= b when I intend a to be modified in place.

Minor points:

There are headers included but not needed for the template:

#include <vector>
#include <iostream>
#include <deque>

inline is implicit for a template function.
We don't need to spell out the full return type for the "append 7" lambda in the example - -> auto& is easier.


First use of std::map in factors and prime factorization program This is from an older question of mine that I decided to revisit:

Simplifying and optimizing factors and prime factorization generator

In that question, someone suggested I use an std::map to hold the factors generated from my inputted number.  I've also decided to use it with my generated prime factorization, which also works.  Based on what I've found online, auto is one way of iterating through an std::map.  I've decided to use that since it appears that my compiler supports C++11.

Am I using std::map appropriately here, or am I developing some bad habits?  I want to develop good habits since I'm interested in using it more in the future.

#include <iostream>
#include <cstdint>
#include <map>
#include <cmath>

void displayFactors(const std::map<uint64_t, uint64_t> &factors);
void displayPrimeFactorization(const std::map<uint64_t, uint64_t> &primeFactorization);
void findFactors(uint64_t, std::map<uint64_t, uint64_t> &factors);
void findPrimeFactorization(uint64_t positiveInt, std::map<uint64_t, uint64_t> &primeFactorization);

int main()
{
    std::map<uint64_t, uint64_t> factors;
    std::map<uint64_t, uint64_t> primeFactorization;
    uint64_t positiveInt;

    std::cout << "\n\n> Positive Int: ";
    std::cin >> positiveInt;

    findFactors(positiveInt, factors);
    std::cout << "\n\n  * Factors:\n\n";
    displayFactors(factors);

    findPrimeFactorization(positiveInt, primeFactorization);
    std::cout << "\n  * Prime Factorization:\n\n      ";
    displayPrimeFactorization(primeFactorization);

    std::cout << std::endl << std::endl;
    std::cin.ignore();
    std::cin.get();
}

void displayFactors(const std::map<uint64_t, uint64_t> &factors)
{
    for (auto iter = factors.begin(); iter != factors.end(); ++iter)
    {
        std::cout << "      " << iter->first << " x " << iter->second << "\n";
    }
}

void displayPrimeFactorization(const std::map<uint64_t, uint64_t> &primeFactorization)
{
    for (auto iter = primeFactorization.begin(); iter != primeFactorization.end(); ++iter)
    {
        if (iter != primeFactorization.begin()) std::cout << " x ";
        std::cout << iter->first;
        if (iter->second > 1) std::cout << '^' << iter->second;
    }
}

void findFactors(uint64_t positiveInt, std::map<uint64_t, uint64_t> &factors)
{
    uint64_t sqrtInt = static_cast<uint64_t>(sqrt(static_cast<double long>(positiveInt)));

    for (uint64_t iter = 1; iter <= sqrtInt; ++iter)
    {
        if (positiveInt % iter == 0)
        {
            uint64_t quotient = positiveInt / iter;
            factors[iter] = quotient;
        }
    }
}

void findPrimeFactorization(uint64_t positiveInt, std::map<uint64_t, uint64_t> &primeFactorization)
{
    uint64_t divisor = 2;
    uint64_t power = 0;

    while (positiveInt != 1)
    {
        while (positiveInt % divisor == 0)
        {
            positiveInt /= divisor;
            power++;
        }

        if (power > 0)
            primeFactorization[divisor] = power;

        power = 0;
        divisor++;
    }
}

Looks generally ok. You can actually shorten iteration even more if your compiler supports range-based for loops:

for (auto iter = factors.begin(); iter != factors.end(); ++iter)
{
    std::cout << "      " << iter->first << " x " << iter->second << "\n";
}

Can be written as:

for(const auto& pair : factors) {
    std::cout << "      " << pair.first << " x " << pair.second << "\n";
}

Further, prefer to return by value than pass by non-const reference:

void findFactors(uint64_t positiveInt, std::map<uint64_t, uint64_t> &factors)

Should be

std::map<uint64_t, uint64_t> findFactors(uint64_t positiveInt)

This will be exactly the same speed in about 98% of situations thanks to (Named) Return Value Optimization.

One final thing to be aware of with map: insertion (or lookup) using [] vs insert.  [] performs a lookup; if the value is not found, it is default-constructed and then the copy-assignment operator (operator=) is called to give the object its value. Thus, [] performs a lookup, construction and copy assignment. It also requires a default constructor to be available. insert, on the other hand does not require a default constructor, and will (from memory) call only the copy constructor. It is thus often (slightly) faster (although for plain old data, the difference will be non-existent). The big difference is not requiring a default constructor, which may not exist.

Finally, a really small thing: it should (technically) be std::uint64_t. Importing <cstdint> (as opposed to stdint.h), based on the C++ spec, will guarantee that this (and uint32_t, etc) will live in the std namespace. It may also put them in the global namespace. Most headers seem to put them in both, but to be 100% portable (according to the standard, anyway), it should be prefixed with a std (or brought into scope with a using std::uint64_t).


Fastest way to "Replace" "auto fill" and then loop in Excel VBA I am wondering if there's any way to improve the performance of this simple "replace" / "autofill" loops that I created in excel?

Sub Macro1()

Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False
Application.EnableEvents = False
Application.DisplayStatusBar = False

For i = 1 To 50
Do

Sheets("Sheet1").Select

'Range("Q1:Q6").Value are random generated number using randbetween (=RANDBETWEEN(3,103))
'save the random created value to Range("U1:U6")
Sheet1.Range("U1:U6").Value = Sheet1.Range("Q1:Q6").Value

Range("AA1").Select

    'Column u1 to u6 are random values generated using =RANDBETWEEN(3,103).
'example of  Range("aa1") Pre-entered formula: =a5+b95+c48+d29+e62+f29
'replace the "a5" in range("aa1") with "a?", with the "?" being the random number in U1.
Selection.Replace What:="a" & Range("t1") & "+", Replacement:="a" & Range("u1") & "+", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

    'replace the "B95" in range("aa1") with "a?", with the "?" being the random number in U2.
Selection.Replace What:="b" & Range("t2") & "+", Replacement:="b" & Range("u2") & "+", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

    'replace the "C48" in range("aa1") with "a?", with the "?" being the random number in U3.
Selection.Replace What:="c" & Range("t3") & "+", Replacement:="c" & Range("u3") & "+", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

    'replace the "D29" in range("aa1") with "a?", with the "?" being the random number in U4.
Selection.Replace What:="d" & Range("t4") & "+", Replacement:="d" & Range("u4") & "+", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

    'replace the "E62" in range("aa1") with "a?", with the "?" being the random number in U5.

Selection.Replace What:="e" & Range("t5") & "+", Replacement:="e" & Range("u5") & "+", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

    'replace the "F29" in range("aa1") with "a?", with the "?" being the random number in U6.
Selection.Replace What:="f" & Range("t6") & ",", Replacement:="f" & Range("u6") & ",", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

'Autofill the modified formula in aa1 to aa1301.
Range("AA1").AutoFill Destination:=Range("AA1:AA1301")
'Manual re-calulate the autofills.
'Manual re-generate the random number

Calculate

    "Copy the number from U1:U6 (the =randbetween(3,103) to T1:T6 to save the random generated number.
'
Sheet1.Range("T1:T6").Value = Sheet1.Range("U1:U6").Value

                         Loop Until Range("ac3") = 1 And Range("h8") >= 8 And Range("o3") = 0 And Range("o4") <> 0

Next i

 Application.Calculation = xlCalculationAutomatic
 Application.ScreenUpdating = True
 Application.EnableEvents = True
 Application.DisplayStatusBar = True

Beep

End Sub

Any better way to speed up the "replace" and the "autofill" or the whole macro even? It's pretty simple, just now fast enough!

Thanks

Just indenting the code and removing the excessive whitespace you have yields something significantly more readable (after also fixing the compile-error with the non-comment after Calculate):

Sub Macro1()

    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.DisplayStatusBar = False

    For i = 1 To 50
        Do
            Sheets("Sheet1").Select

            'Range("Q1:Q6").Value are random generated number using randbetween (=RANDBETWEEN(3,103))
            'save the random created value to Range("U1:U6")
            Sheet1.Range("U1:U6").Value = Sheet1.Range("Q1:Q6").Value

            Range("AA1").Select

            'Column u1 to u6 are random values generated using =RANDBETWEEN(3,103).
            'example of  Range("aa1") Pre-entered formula: =a5+b95+c48+d29+e62+f29
            'replace the "a5" in range("aa1") with "a?", with the "?" being the random number in U1.
            Selection.Replace What:="a" & Range("t1") & "+", Replacement:="a" & Range("u1") & "+", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'replace the "B95" in range("aa1") with "a?", with the "?" being the random number in U2.
            Selection.Replace What:="b" & Range("t2") & "+", Replacement:="b" & Range("u2") & "+", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'replace the "C48" in range("aa1") with "a?", with the "?" being the random number in U3.
            Selection.Replace What:="c" & Range("t3") & "+", Replacement:="c" & Range("u3") & "+", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'replace the "D29" in range("aa1") with "a?", with the "?" being the random number in U4.
            Selection.Replace What:="d" & Range("t4") & "+", Replacement:="d" & Range("u4") & "+", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'replace the "E62" in range("aa1") with "a?", with the "?" being the random number in U5.
            Selection.Replace What:="e" & Range("t5") & "+", Replacement:="e" & Range("u5") & "+", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'replace the "F29" in range("aa1") with "a?", with the "?" being the random number in U6.
            Selection.Replace What:="f" & Range("t6") & ",", Replacement:="f" & Range("u6") & ",", LookAt:=xlPart, _
                              SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
                              ReplaceFormat:=False

            'Autofill the modified formula in aa1 to aa1301.
            Range("AA1").AutoFill Destination:=Range("AA1:AA1301")
            'Manual re-calulate the autofills.
            'Manual re-generate the random number

            Calculate

            'Copy the number from U1:U6 (the =randbetween(3,103) to T1:T6 to save the random generated number.
            Sheet1.Range("T1:T6").Value = Sheet1.Range("U1:U6").Value

        Loop Until Range("ac3") = 1 And Range("h8") >= 8 And Range("o3") = 0 And Range("o4") <> 0
    Next i

    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.DisplayStatusBar = True

    Beep
End Sub

Let's start with this.

The next step is breaking this into more manageable chunks:

Private Sub EnablePerformanceOptimizations()
    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.DisplayStatusBar = False
End Sub

Putting this together with the other block at the end of your current Macro1 we get:

Private Sub PerformanceOptimizations(ByVal enable As Boolean)
    Application.Calculation = IIf(enable, xlCalculationManual, xlCalculationAutomatic)
    Application.ScreenUpdating = Not enable
    Application.EnableEvents = Not enable
    Application.DisplayStatusBar = Not enable
End Sub

Now that this is finished we can "reimagine" Macro1 as the following

Public Sub Macro1()
    PerformanceOptimizations(True)

    GenerateAndCopyRandomNumbers()

    PerformanceOptimizations(False)
End Sub

Suddenly we can see "exactly" what Macro1 does. It does performance optimizations before generating random numbers and then disables the optimizations again.

The next step is to optimize that number generation.

The code you posted here does never use the loop variable of the For Next loop. We can just completely drop that loop, which already should reduce the runtime to \$\frac{1}{50}\$ of the current runtime...

The next step is optimizing these Replace calls.

The first step towards that would usually be to get rid of Selection because that's slow. I'll ignore that for now and just reformat the code (again) just to see what's actually going on:

Private Sub GenerateAndSaveRandomNumbers()
    Do
        Sheets("Sheet1").Select

        'Range("Q1:Q6").Value are random generated number using randbetween (=RANDBETWEEN(3,103))
        'save the random created value to Range("U1:U6")
        Sheet1.Range("U1:U6").Value = Sheet1.Range("Q1:Q6").Value

        Range("AA1").Select

        'Column u1 to u6 are random values generated using =RANDBETWEEN(3,103).
        'example of  Range("aa1") Pre-entered formula: =a5+b95+c48+d29+e62+f29
        'replace the "a5" in range("aa1") with "a?", with the "?" being the random number in U1.
        Selection.Replace What:="a" & Range("t1") & "+", _
                          Replacement:="a" & Range("u1") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'replace the "B95" in range("aa1") with "a?", with the "?" being the random number in U2.
        Selection.Replace What:="b" & Range("t2") & "+", _
                          Replacement:="b" & Range("u2") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'replace the "C48" in range("aa1") with "a?", with the "?" being the random number in U3.
        Selection.Replace What:="c" & Range("t3") & "+", _
                          Replacement:="c" & Range("u3") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'replace the "D29" in range("aa1") with "a?", with the "?" being the random number in U4.
        Selection.Replace What:="d" & Range("t4") & "+", _
                          Replacement:="d" & Range("u4") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'replace the "E62" in range("aa1") with "a?", with the "?" being the random number in U5.
        Selection.Replace What:="e" & Range("t5") & "+", _
                          Replacement:="e" & Range("u5") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'replace the "F29" in range("aa1") with "a?", with the "?" being the random number in U6.
        Selection.Replace What:="f" & Range("t6") & ",", _
                          Replacement:="f" & Range("u6") & ",", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False, _
                          SearchFormat:=False, ReplaceFormat:=False

        'Autofill the modified formula in aa1 to aa1301.
        Range("AA1").AutoFill Destination:=Range("AA1:AA1301")
        'Manual re-calulate the autofills.
        'Manual re-generate the random number

        Calculate

        'Copy the number from U1:U6 (the =randbetween(3,103) to T1:T6 to save the random generated number.
        Sheet1.Range("T1:T6").Value = Sheet1.Range("U1:U6").Value

    Loop Until Range("ac3") = 1 And Range("h8") >= 8 And Range("o3") = 0 And Range("o4") <> 0
End Sub()

Now since Replace has quite a bunch of optional arguments, and you're overwriting most of them with defaults or unhelpful values, let's drop them:

MatchCase defaults to False
MatchByte isn't used in your code
SearchFormat would boil down to a Falsey if you didn't pass it
ReplaceFormat same considerations as for SearchFormat apply.

Interestingly we can even go so far as to abuse the fact that LookAt, SearchOrder and MatchCase (as well as MatchByte) are "saved" between invocations to collapse all this into the following replacement calls:

        'Column u1 to u6 are random values generated using =RANDBETWEEN(3,103).
        'example of  Range("aa1") Pre-entered formula: =a5+b95+c48+d29+e62+f29
        'replace the "a5" in range("aa1") with "a?", with the "?" being the random number in U1.
        Selection.Replace What:="a" & Range("t1") & "+", _
                          Replacement:="a" & Range("u1") & "+", LookAt:=xlPart, _
                          SearchOrder:=xlByRows, MatchCase:=False

        'replace the "B95" in range("aa1") with "a?", with the "?" being the random number in U2.
        Selection.Replace What:="b" & Range("t2") & "+", _
                          Replacement:="b" & Range("u2") & "+"

        'replace the "C48" in range("aa1") with "a?", with the "?" being the random number in U3.
        Selection.Replace What:="c" & Range("t3") & "+", _
                          Replacement:="c" & Range("u3") & "+"

        'replace the "D29" in range("aa1") with "a?", with the "?" being the random number in U4.
        Selection.Replace What:="d" & Range("t4") & "+", _
                          Replacement:="d" & Range("u4") & "+"

        'replace the "E62" in range("aa1") with "a?", with the "?" being the random number in U5.
        Selection.Replace What:="e" & Range("t5") & "+", _
                          Replacement:="e" & Range("u5") & "+"

        'replace the "F29" in range("aa1") with "a?", with the "?" being the random number in U6.
        Selection.Replace What:="f" & Range("t6") & ",", _
                          Replacement:="f" & Range("u6") & ","

Continued simplfications include not using Selection and using a With block:

With Sheet1.Range("AA1")
    .Replace What:="a" & Range("t1") & "+", _
             Replacement:="a" & Range("u1") & "+", LookAt:=xlPart, _
             SearchOrder:=xlByRows, MatchCase:=False
    'replace the "B95" in range("aa1") with "a?", with the "?" being the random number in U2.
    .Replace What:="b" & Range("t2") & "+", Replacement:="b" & Range("u2") & "+"

    'replace the "C48" in range("aa1") with "a?", with the "?" being the random number in U3.
    .Replace What:="c" & Range("t3") & "+", Replacement:="c" & Range("u3") & "+"

    'replace the "D29" in range("aa1") with "a?", with the "?" being the random number in U4.
    .Replace What:="d" & Range("t4") & "+", Replacement:="d" & Range("u4") & "+"

    'replace the "E62" in range("aa1") with "a?", with the "?" being the random number in U5.
    .Replace What:="e" & Range("t5") & "+", Replacement:="e" & Range("u5") & "+"

    'replace the "F29" in range("aa1") with "a?", with the "?" being the random number in U6.
    .Replace What:="f" & Range("t6") & ",", Replacement:="f" & Range("u6") & ","

This should give you another performance boost and will continue to work, even if the user changes the Selection during the execution of the macro.


Normalization, division and reshaping processes on an HDF5 file I performing a set of division operations and reshaping on a HDF5 file. As my datapoints is around 4000 in number it takes around a minute. I will be adding more data, which will further slow my overall code execution time.
How can I optimize this code to make it faster?

def pre_proc_data():
    jointMatrix = np.array([], dtype=np.float64).reshape(0, 500 * 17)
    hdf5_file = h5py.File("/home/Data.hdf5")

    for j in range(len(hdf5_file["vector"])):
        # Normalization
        norm_vec = hdf5_file["vector"][j]
        norm_vec[:, 0] = (norm_vec[:, 0] - (-3.059)) / 6.117  # W0 - Left and right
        norm_vec[:, 5] = (norm_vec[:, 5] - (-3.059)) / 6.117
        norm_vec[:, 1] = (norm_vec[:, 1] - (-1.5707)) / 3.6647  # W1
        norm_vec[:, 6] = (norm_vec[:, 6] - (-1.5707)) / 3.6647
        norm_vec[:, 2] = (norm_vec[:, 2] - (-3.059)) / 6.117  # W2
        norm_vec[:, 14] = (norm_vec[:, 14] - (-3.059)) / 6.117
        norm_vec[:, 3] = (norm_vec[:, 3] - (-1.7016)) / 3.4033  # S0
        norm_vec[:, 10] = (norm_vec[:, 10] - (-1.7016)) / 3.4033
        norm_vec[:, 4] = (norm_vec[:, 4] - (-2.147)) / 3.194  # s1
        norm_vec[:, 8] = (norm_vec[:, 8] - (-2.147)) / 3.194

        norm_vec[:, 11] = (norm_vec[:, 11] - (-3.0541)) / 6.1083  # eo
        norm_vec[:, 15] = (norm_vec[:, 15] - (-3.0541)) / 6.1083
        norm_vec[:, 12] = (norm_vec[:, 12] - (-0.05)) / 2.67  # e1
        norm_vec[:, 16] = (norm_vec[:, 16] - (-0.05)) / 2.67

        reshaped_vec = hdf5_file["vector"][j].reshape(500 * 17)
        jointMatrix = np.vstack((jointMatrix, reshaped_vec))

    return jointMatrix

jointMatrix = pre_proc_data()

It seems that all your code could be vectorized with the help of numpy broadcasting.

At first, instead of using all these norm_vec[:, ...] = ... you could create two vectors of length 17 containing values you use to normalize the data.

I assume normalizing values to be mean and standard deviation (please tell me if I'm wrong), so I'll be calling them mean and std correspondingly.

mean is a np.ndarray with values [-3.059, -1.5707, ..., -0.05] and std is a np.ndarray with values [6.117, 3.6647, ..., 2.67] (indicies ranging from zero to 16).

Using this notation, for loop could be rewritten:

for j in range(len(hdf5_file["vector"])):

        norm_vec = (hdf5_file["vector"][j] - mean) / std

        reshaped_vec = norm_vec.reshape(500 * 17)
        jointMatrix = np.vstack((jointMatrix, reshaped_vec))

This should give a certain speed-up. However, the code could be further optimized by vectorizing the loop itself.

The whole code now looks like this:

def pre_proc_data():

    hdf5_file = h5py.File("/home/Data.hdf5")
    norm_vec = (hdf5_file["vector"] - mean) / std
    # from 3d to 2d
    return norm_vec.reshape(-1, 500 * 17)

jointMatrix = pre_proc_data()


Copying cells from one sheet to another I'm new to writing VBA and having spent most of the day writing some code, I have finally got it working but it looks horrible. I'm hoping that someone can help me tidy it up.

The code simply copies certain cells from one sheet into another. If a particular cell in the source sheet contains the word "Accepted". Columns A of the source and destination sheets contain a unique reference and that data is only copied across if the unique reference is not already in the destination data. In addition, my code adds the date at which each entry is made in the destination sheet.

Ideally I need the code to look tidy and professional and probably most importantly be understandable to anyone looking at it, whether that be someone else or me in a years time - when I've forgotten what I did.

Public Sub UpdateAcceptedChangeRequests()

With Application
    .ScreenUpdating = False
    .EnableEvents = False
End With

On Error GoTo errorHandler:

Dim varAnswer As String
    varAnswer = MsgBox("Are you sure you want to update the Accepted Change Requests List?", vbYesNo, "Update Accepted Change Requests")
        If varAnswer = vbNo Then
            MsgBox ("No changes saved")
                With Application
                    .ScreenUpdating = True
                    .EnableEvents = True
                End With
            Exit Sub
        End If

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
    Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
    Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

        LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
        LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

            For i = 2 To LastRowSourceSheet
                If SourceSheet.Range("E" & i).Value = "Accepted" Then
                    If Evaluate("ISERROR(MATCH(A" & i & ",'Accepted Change Requests'!A:A,0))") Then
                        DestSheet.Range("A" & LastRowDestSheet + 1 & ":D" & LastRowDestSheet + 1).Value = _
                            SourceSheet.Range("A" & i & ":D" & i).Value
                            With DestSheet.Range("E" & LastRowDestSheet + 1)
                                .Value = Date
                                .NumberFormat = "dd/mm/yyyy"
                            End With
                        LastRowDestSheet = LastRowDestSheet + 1
                    End If
                End If
            Next i

        With Application
        .ScreenUpdating = True
        .EnableEvents = True
        End With

Exit Sub

errorHandler:
MsgBox ("There was an error adding this Change Request")
Resume Next

With Application
    .ScreenUpdating = True
    .EnableEvents = True
End With

End Sub

When you'll look at this code a number of years months (weeks?) from now, the first thing that will jump at you and make you want to rip your eyes off, is the indentation.

The idea is that code blocks add a level. What's a code block you'll ask? A Sub or a Function defines a scope - as such, the only thing you should see at the same indentation level as the words Public Sub, is the words End Sub - and labels, because the VBE wants them unindented.

Declaring a variable does not constitute a code block. These lines should all be at the same indentation level:

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
    Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
    Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

        LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
        LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

            For i = 2 To LastRowSourceSheet

Like this:

Dim SourceRange As Range, DestRange As Range
Dim DestSheet As Worksheet, SourceSheet As Worksheet
Dim LastRowDestSheet As Long, i As Long, LastRowSourceSheet As Long
Set DestSheet = ThisWorkbook.Worksheets("Accepted Change Requests")
Set SourceSheet = ThisWorkbook.Worksheets("All Change Requests")

LastRowDestSheet = DestSheet.Cells(DestSheet.Rows.Count, "A").End(xlUp).Row
LastRowSourceSheet = SourceSheet.Cells(SourceSheet.Rows.Count, "E").End(xlUp).Row

For i = 2 To LastRowSourceSheet

I would avoid declaring multiple variables in a single instruction. You've done it right though - every variable has a type and that's good (common beginner mistake is to only specify a type for the last one, making all but the last one Variant variables)... but things are simply easier to read (and to maintain!) when each declaration has its Dim statement.

A With block however, is a block, and as such warrants an indentation level. Same with For and If blocks, which you all got right.

The parentheses are not needed here, and can play bad tricks on you if you make it a habit:

MsgBox ("There was an error adding this Change Request")

Method calls in VBA don't require parentheses - these brackets actually force an expression evaluation, that's all they're doing. And in other contexts this forced evaluation can introduce hard-to-find bugs.

Good error messages have proper punctuation, but the problem isn't much with the message, than with how the error is handled:

Resume Next

This takes execution back to the line after the one that blew up. Normally you resume next when you know you've recovered from the error and that everything is stable enough to just resume with the next instruction - it's not the case here. This handler is basically On Error Resume Next with a message box between the error and the resume next. Bad things can happen - if an error occurred and you don't know what caused it, nor whether you can actually recover from it, it's usually best to simply exit the procedure, aborting the process.

Note that in VBA the colon (:) is an instructions separator (except when it's identifying a label), used for stuffing multiple instructions on the same line. Hence, it's completely superfluous here:

On Error GoTo errorHandler:

Because the "next instruction" is a line terminator, a non-instruction.

I merely scratched the surface here, there's a lot to say about this code - I'll leave some for other reviewers :)


Add / Overwrite an object in an array I am trying to overwrite an object in an array if the title property exists, otherwise just push it to the array. I found two approaches and I wonder which one is the preferred one.
Performance is not really and issue but I wonder whether mutability could be, or just there is a better way to do this altogether.
On this snippet I am using a for loop to edit the original array:

const data = [
  {
    title: 'AAA',
    people: [ 'John', 'Megan',]
  },{
    title: 'BBB',
    people: [ 'Emily', 'Tom']
  }
]

// If inputTitle is not on any of data's title it will append not overwritte
const inputTitle = 'AAA'
const inputPeople = ['Peter', 'Jane']

for (const obj of data) {
  if (obj.title === inputTitle) {
    obj.people = inputPeople
    break
  } else {
    data.push({
      title: inputTitle,
      people: inputPeople
    })
    break
  }
}

console.log(data)

Here I am using high order functions and spread to do the same:

const data = [
  {
    title: 'AAA',
    people: [ 'John', 'Megan',]
  },{
    title: 'BBB',
    people: [ 'Emily', 'Tom']
  }
]

// If inputTitle is not on any of data's title it will append not overwritte
const inputTitle = 'AAA'
const inputPeople = ['Peter', 'Jane']

let res = []

if (data.some(({ title }) => title === inputTitle)) {
  res = data.map(obj => {
    if (obj.title === inputTitle)
      obj.people = inputPeople
    return obj
  })
} else {
  res = [...data, { title: inputTitle, people: inputPeople}]
}

console.log(res)

In the real task I am reading the data array from a json file with node and writing the changes back to it.

From a short review;

The first snippet does not work, try it with inputTitle = 'BBB'

You are in essence doing 2 things

Find a possible match
Update the match or insert the data

Ideally, this is done in 1 'loop'

If you don't care too much about performance, I would go for a functional approach

This is my counter-proposal (which modifies the data)

const data = [
  {
    title: 'AAA',
    people: [ 'John', 'Megan',]
  },{
    title: 'BBB',
    people: [ 'Emily', 'Tom']
  }
]

function mergePeople(data, title, people){
  const target = data.filter(record=>record.title===title);
  if(target.length){
    target.forEach(record => record.people = people);
  }else{
    data.push({title, people});
  }
  return data;
}

function quickClone(o){
  return JSON.parse(JSON.stringify(o));
}

console.log(mergePeople(quickClone(data), "AAA", ["Tom", "Jerry"]));
console.log(mergePeople(quickClone(data), "BBB", ["Ben", "Jerry"]));


Find cells with specific formatting I'm currently writing code so that when a red-colored cell appears in a column, a message box will appear letting me know that column has an error. I have written the following method:

Sub MSG()

    Dim a
    Dim B

    With Application.FindFormat.Interior
        .color = vbRed
    End With

    '/ Sheet1 is example sheet name
    Set a = Sheet1.Columns(1).Find(What:="", SearchFormat:=True)
    Set B = Sheet1.Columns(2).Find(What:="", SearchFormat:=True)

    If Not a Is Nothing Then
          MsgBox ("There is an issue with column A, please review.")
    End If
    If Not B Is Nothing Then
          MsgBox ("There is an issue with column B, please review.")
    End If

End Sub

But as you can see, expanding this to support an arbitrary number of columns would be impossible, and going up to column ED would require 134+ lines of code to be written!

Is there a more efficient way to do this? Would it be possible to optimize this code?

The basic construct you're looking for is a loop. This is what you should reach for any time you find yourself writing the same code (performing the same operation) over and over again, but in a regular pattern (e.g., on columns A–ED).

There are several different types of loops supported by VBA (consult your favorite language reference for details), but you probably want a ranged loop here, since you want to loop through columns A through ED (or whatever maximum column). In VBA parlance, that would be a For loop; something like this:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For col = 1 To 134
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & col & "; please review."
        End If
    Next
End Sub

Notice that I have made the following additional improvements to your original code:

I have replaced MSG with a meaningful and descriptive name for the subroutine. This might not be the perfect name, but it is certainly a major improvement. It's also less likely to conflict with other identifiers (msg is pretty common, and VBA isn't case-sensitive).
The With…EndWith construct is only useful when you are setting multiple properties at a particular level of nesting. Your With block only had one statement inside of it, so I've removed it and replaced it with a single statement.
I believe variables should be declared as close as possible to the point of use. Ideally, they would be declared and initialized all at once, but if I remember correctly, VBA doesn't actually support this; you have to break it up into two separate lines. Still, keep 'em close! Also, keep them as narrowly scoped as possible—notice that here, I've put the variable declaration inside of the loop, instead of outside.
If a subroutine takes only one argument, you should not wrap its argument list in parentheses. Thus, I've removed the parentheses around the string passed in the call to the MsgBox subroutine.

But the code I've written above isn't quite what you want—we still need to fix a couple more things!

The first issue is that it will display a message box containing the numeric ID of the problematic column, rather than the alphabetic ID displayed in the Excel UI. For your user's sake, you probably want to change this. Since you're using a loop with the numeric column ID as the loop counter, you need an algorithm to systematically convert that numeric ID to the alphabetic ID. I would define a reusable function for this. The following function is adapted from one appearing in a Microsoft support article:

Function ColumnNumberToLetter(col As Integer) As String
    Dim alpha As Integer
    alpha = col \ 27

    Dim remainder As Integer
    remainder = col - (alpha * 26)

    If alpha > 0 Then
        ColumnNumberToLetter = Chr(alpha + 64)
    End If

    If remainder > 0 Then
        ColumnNumberToLetter = ColumnNumberToLetter & Chr(remainder + 64)
    End If
End Function

Then, just plug in a call to this function in the appropriate spot:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For col = 1 To 134
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

Also, now that we've got a loop, we no longer have to arbitrarily limit ourselves to going through column ED. It is simple to tweak this code to support any number of columns in a sheet—you just make the loop's upper bound the sheet's last column:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    Dim totalColumns As Integer
    totalColumns = Sheet1.Columns.Count

    For col = 1 To totalColumns
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

In fact, though, we can do even better. VBA supports a special type of For loop that can be used to automatically iterate through an entire collection of values (in this case, all of the columns in a sheet). This is the For Each loop, and it can be used like so:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For Each col In Sheet1.Columns
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

This is much clearer, less error-prone (it no longer makes assumptions about what the first column is), and more extensible. Almost perfect!

One last thing—instead of hard-coding which sheet you want this function to work on, consider passing in the sheet as an argument:

Sub ShowMsgForErrorColumns(sheet As Worksheet)
    Application.FindFormat.Interior.Color = vbRed

    For Each col In sheet.Columns
        Dim match
        Set match = sheet.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

That makes this function reusable. You don't want to go too far with reusability (YAGNI), but at the same time, you don't want to hard-code assumptions or arbitrarily box yourself in. Sometimes, it's just more natural to parameterize a function.

The only thing that bothers me about this code is that setting the Application.FindFormat.Interior.Color property has global side-effects that are visible outside of the function. This bothers me for the same reason that global variables make my skin crawl. One way to solve this would be to save the original value at the top of the function, and then restore it at the end. But that might not be worth it, if you are disciplined about always making sure to set this property to the desired value before any calls to the Find function.


Exchange system of numbers I've got the following task:

  Keeping in mind Horner's Scheme, write an application that converts a given number x in n-number system to m-number system where m, n <= 10. x is a natural number or zero, x <= unsigned(-1)

My code is as follows:

#include <iostream>

int toDecimal (int, int);
int fromDecimal (int, int);
int convert(int, int, int);

int main()
{
    int base, number, desiredBase;
    std::cin >> number >> base >> desiredBase;
    std::cout << convert(number, base, desiredBase) << std::endl;
}

int toDecimal (int base, int number)
{
    if (number / 10 == 0) {
        return number;
    }
    return (number % 10) + (base * toDecimal (base, number / 10));
}

int fromDecimal (int base, int number)
{
    if (number / base == 0) {
        return number;
    }
    return (number % base) + (10 * fromDecimal (base, number / base));
}

int convert(int number, int base, int desiredBase)
{
    int p = toDecimal(base, number);
    return fromDecimal(desiredBase, p);
}

What can I improve? What can be done better?

In general, the code is well presented, easy to read, but, it is hard to spot the 'Horner's Scheme' in the code.

When implementing a specific algorithm in code, it is useful to clearly comment where the elements of the algorithm are being used.

In this case, the depth-first recursion is processing the most significant digits first, and as a result, it computes the high-order values in the Horner's scheme first. Making that value available to be added to the next value in the system.

Note that this makes tail-recursion optimization impossible, but it does simplify the code.

So, I had to look up how Horner's scheme would help your code, and I had to figure out how your code is helped by it. This is not work that should be hard. You should make that easy for the person reading the code.

I would expect something like:

// based on Horner's scheme: http://en.wikipedia.org/wiki/Horner%27s_method
// The source base of the value can be considered to be Xo in the algorithm, and
// the digit value is the coefficient for that base.

As for your recursion, you can simplify it a little by recursing one level more, and returning 0 (eliminating a duplicated division on each level). Consider your code:

int toDecimal (int base, int number)
{
    if (number / 10 == 0) {
        return number;
    }
    return (number % 10) + (base * toDecimal (base, number / 10));
}

and replacing that code with:

int toDecimal (int base, int number)
{
    if (number == 0) {
        return 0;
    }
    return (number % 10) + (base * toDecimal (base, number / 10));
}

The difference is marginal, trading one division/comparison with a simple comparison and an extra level of recursion.

Regardless, I prefer the reduced code duplication, and it makes the recursion termination easier to see.

The other item I see missing is validation on the input. I would prefer to see some exceptions thrown if the input is in a base that does not support the supplied digits. For example, with the input:

12345 4 10

Putting this all together, I suggest the following:

#include <iostream>
#include <stdexcept>

int toDecimal (int, int);
int fromDecimal (int, int);
int convert(int, int, int);

int main()
{
    int base, number, desiredBase;
    std::cin >> number >> base >> desiredBase;
    try
    {
        std::cout << convert(number, base, desiredBase) << std::endl;
    } catch (const std::invalid_argument& e)
    {
        std::cerr << "Unable to convert " << number << " from base " << base << std::endl;
        return 1;
    }
}

int toDecimal (int base, int number)
{
    if (number == 0)
    {
        return 0;
    }

    int digit = number % 10;
    if (digit >= base)
    {
        throw std::invalid_argument( "received out-of-range digits in the input for the supplied base");
    }

    return digit + (base * toDecimal (base, number / 10));
}

int fromDecimal (int base, int number)
{
    if (number == 0) {
        return 0;
    }
    return (number % base) + (10 * fromDecimal (base, number / base));
}

int convert(int number, int base, int desiredBase)
{
    int p = toDecimal(base, number);
    return fromDecimal(desiredBase, p);
}

In addition to handling the exceptions from invalid input numbers, you should also handle requests to/from invalid bases as well (like negative or bases > 10).


Making a dynamic parser to collect data from a javascript enabled webpage I've written a scraper in VBA in combination with selenium to parse product names and prices from a javascript enabled webpage. The thing is that I just used selenium to get the page source (as it was not possible to get the response text without opening the page with selenium) and as soon as I get it I reverted back to usual VBA method to complete the operation.

Most importantly, it was hard to deal with parsing prices cause there are two types of class names for prices so if a particular class name is used in the script then it is noticed that, after scraping is done, some fields are blank in the price section in the spreadsheet.

However, I have handled the two class names effectively in my parser to get prices flawlessly. Now, it is working great and way faster than selenium alone. If there is any improvement I can make in this script, I would be very happy to comply. Thanks in advance.

Here is what I've written:

Sub RedmartScraper()
  Dim driver As New ChromeDriver, html As New HTMLDocument
  Dim post As HTMLHtmlElement

  With driver
    .Get "https://redmart.com/bakery"
    html.body.innerHTML = .ExecuteScript("return document.body.innerHTML;")
    .Quit
  End With

  For Each post In html.getElementsByClassName("productDescriptionAndPrice")
    With post.getElementsByTagName("h4")(0).getElementsByTagName("a")
      If .Length Then i = i + 1: Cells(i, 1) = .item(0).innerText
    End With

    With post.getElementsByClassName("ProductPrice__promo_price___3OWY9")
        If .Length Then Cells(i, 2) = .item(0).innerText
    End With

    With post.getElementsByClassName("ProductPrice__price___3BmxE")
        If .Length Then Cells(i, 2) = .item(0).innerText
    End With
  Next post
End Sub

Btw, libraries to add in the reference library:

Microsoft Html Object Library
Selenium Type Library

One of the potential disadvantages of HTML-parsing vs interacting with the page via Selenium API is that you would not be able to examine the current state of the page - e.g. determine which elements are visible, get only visible text of an element, click buttons and links, execute JavaScript. In other words, you would miss all the dynamic behaviors.

But, on the other hand, every selenium command is an HTTP request - JSON over HTTP which introduces tons of overhead. And, if your page is completely loaded and you just need to only extract the data itself, then it does make perfect sense to avoid going through selenium API.

I would only fix a couple of things in your current implementation:

I don't particularly like the ___3OWY9 and ___3BmxE in the class names - these parts look dynamically generated and it feels like this can change. I would use a CSS selector and a partial match on a class attribute:

With post.querySelector("[class^=ProductPrice__promo_price]")

and

With post.querySelector("[class^=ProductPrice__price]")

and I would also use a CSS selector for getting the product description:

With post.querySelector("h4 > a")

Note that querySelector would return a single element - hence, you would not need (0) part after the item.