Linked list in JavaScript I wrote this simple linked list in JavaScript. Feedback is welcome.

function Node(data)
{
    this.next = null;
    this.data = data;
}

function LinkedList()
{
    this.length = 0;
    this.head = null;

    // add node with given value to the list.
    this.add = function (value)
    {
        var node = new Node(value);

        var temp;

        if(this.length == 0)
        {
            this.head = node;
            this.length++;
            return;
        }

        temp = this.head;

        // Move to the position where we can perform addition
        // This logic is slightly different when we for example need to do search.
        while(temp.next)
        {
            temp = temp.next;
        }

        temp.next = node;
        this.length++;

        return;

    }

    // Search for node with given value.
    this.search = function (value)
    {
        // index where the node was found
        var index = 0;

        // If the list is empty there is no point in searching.
        if(!this.head)
        {
            console.log("List is empty");
            return;

        }

        var temp = this.head;

        while(temp)
        {
            if(temp.data == value)
            {
                console.log("Found at: "  + index);
                return;
            }

            // move to next node
            temp = temp.next

            index++;
        }

        console.log("Node not found");

    }

    // Dump whole list
    this.print = function()
    {
        if(!this.head)
        {
            console.log("List is empty");
            return;

        }

        var temp = this.head;

        while(temp)
        {
            console.log(temp.data);
            temp = temp.next
        }

    }

    // Remove node at index. Index starts from 0.
    this.removeAtIndex = function (index)
    {
        var i = 0;

        if(index < 0 || index >= this.length)
            throw "Wrong index";

        var temp = this.head;

        if(!this.head)
            return;

        if(index == 0)
        {
            this.head = this.head.next;
            this.length--;
            return;
        }

        // Move to the position where we can perform delete.
        for(i = 0; i < index  - 1; i++)
        {
            temp = temp.next;
        }

        temp.next = temp.next.next;
        this.length--;
        return;

    }
}

var x = new LinkedList();
x.add(200);
x.add(100);
x.add(300);
x.add(400);
x.print();
x.search(400);
x.removeAtIndex(2);
x.print();
x.search(400);

JavaScript has an array data type that acts like different data structures. It can be a list, a stack, or even a queue depending on how you use it. If this code is merely for learning purposes, then it's fine. Otherwise, try using available native data structures instead of reinventing them.

Now to your code. First off, I would suggest against using constructors and use basic object literals and functions instead. It is simpler to think of data simply as a structure of arrays and objects.

this.add

Very vague name. It's not clear whether we're adding a new node (where we pass in a new instance of Node) or adding a value (that the method will wrap for us, which it currently does). It's best if you make it more verbose, like addValue.

if(this.length == 0)

length is a function of the number of nodes in the list. A common problem I had in data structures class is if the length value was out of sync with the actual node count value. This made very interesting bugs when length and the actual node count didn't match up.

To get the length, it's better, although inefficient, to actually count the nodes present. Alternatively, a list without a head is essentially an empty list. if(!this.head) is sufficient to determine an empty list.

temp = this.head;
while(temp.next){
  temp = temp.next;
}

This can be extracted to a findTail function which can be really useful. Alternatively, keeping track of the tail like you're doing with head is also a convenient and more efficient way to do this. Not sure if there are standards to linked lists, but it's an optimization you can take advantage.

this.search = function (value){}

I don't really get the point of search. Is it supposed to just print a message if it exists? If it does, it's better named like printMessageIfValueExists. Otherwise, one would take it as a function that takes a value and returns a boolean representing its existence.

Also, a linked list best remain a data structure. It shouldn't handle printing as this makes it dependent on printing capabilities of the environment, an unnecessarily coupled dependency. Leave that to whatever code is using the linked list. Same goes for your print method.

this.removeAtIndex = function (index)

Again, another one of those vaguely-named functions. Why? It's because we're not sure what "remove" exactly means. It could mean removing the value or removing the node entirely. I see that it's removing a node, so it should be removeNodeAtIndex.


DataTable runtime grouping on user defined criteria I one of my tools I use the user can define grouping for a DataTable. The criteria are known only at run-time. To achieve this I use a Dictionary<string, object> with a custom dictionary comparer that the GroupBy extension cosumes so that I can aggregate the groups. This works pretty well but I was wondering if there is still room for improvement.

The number and names of columns can change. There is nothing known about the DataTable until a JSON configuration is loaded. This is an example of such a configuration from one of my unit tests.

"Columns": [
    "_nvarchar | key",
    "_datetime2",
    "_int | sum",
    "_float | avg",
    "_bit | count",
    "_money",
    "_numeric"
],

(The configuration is already being taken care of and is not a part of the review, it's just for reference to show how the columns can be configured.)

To make it simpler here's some example data:

var dataTable = new DataTable();
dataTable.Columns.Add("FirstName", typeof(string));
dataTable.Columns.Add("LastName", typeof(string));
dataTable.Columns.Add("ItemCount", typeof(int));

dataTable.Rows.Add("foo", "bar", 2);
dataTable.Rows.Add("foo", "bar", 5);
dataTable.Rows.Add("baz", "qux", 3);
dataTable.Rows.Add("foo", "bar", 2);
dataTable.Rows.Add("baz", "qux", 6);

The keyColumns are those marked with key - they are not the same as database keys and they are for aggregation later.

var keyColumns = new[] { "FirstName", "LastName" };

The implementation is very simple. Just enumerate the rows and put the name of the key column in the dictionary's key and the value of that column as the value of that dictionary item.

This is exactly the implementation I currently use:

var rowGroups = dataTable.AsEnumerable().GroupBy(x =>
    keyColumns.ToDictionary(
        column => column,
        column => x[column],
        StringComparer.OrdinalIgnoreCase
    ),
    new DictionaryComparer<string, object>()
).ToList();

To actually be able to group the dictionaries there is also this comparer:

public class DictionaryComparer<TKey, TValue> : IEqualityComparer<IDictionary<TKey, TValue>>
{
    public bool Equals(IDictionary<TKey, TValue> x, IDictionary<TKey, TValue> y)
    {
        return x.All(item => y.TryGetValue(item.Key, out TValue value) && item.Value.Equals(value));
    }

    // Ignore dictionary hash-code and just compare the keys and values.
    public int GetHashCode(IDictionary<TKey, TValue> obj) => 0;
}

And the result is:

Honestly, I am not sure if a dictionary is the right data structure for realizing composed keys. At least for me it is not directly obviously how the data are grouped. I think the main issue that confuses me is, that the grouping key contains not just the values to group by, but also the column headers which is absolutely irrelevant for grouping. Additionally I have to understand the comparer.

For your example, you could easily use a tuple which provides build-in comparison of each item:

var rowGroups = dataTable.AsEnumerable()
    .GroupBy(x => Tuple.Create(x["FirstName"], x["LastName"]))
    .ToList();

Another more flexible approach could be to create a string value to be used as grouping key:

var rowGroups = dataTable.AsEnumerable()
    .GroupBy(x => string.Join(":", keyColumns.Select(key => x[key])))
    .ToList();


How to write efficient javascript/jQuery code involving loops and string concat? I am using kendo grid's onDataBound event to do something.  Here's my jQuery code:

console.time('withoutnot');
jQuery(e.sender.dataSource._data).each(function (i, v) {
     if (v.IsReadonly) {
         e.sender.wrapper.find("tr[data-uid='" + v.uid + "'] .k-button")
                .each(function (i, j) {
                    if (!jQuery(this).hasClass('k-grid-edit') && !jQuery(this).hasClass('k-grid-Desc')) {
                        jQuery(this).addClass('k-state-disabled').prop('disabled', true)
                                .prop('title', 'Readonly.');
                    }
                });
    }
});
console.timeEnd('withoutnot');

onDataBound event gets raised once the grid has completed data-load from serverside.

My problem is that, because of 400 rows in the grid (without paging), it takes around 355ms to 378ms, which is still slower for me, because it causes visible performance lag in the grids load(readonly button effect).

How can I improve this code snippet even more? or is it the best I can go for?

NOTE: using native for or jQuerys :not (instead of those if conditions), proved disaster to me, load time scaled to 460ms to 550ms.

I am using jQuery 1.7.1

I found out solution to my problem. so basically this is what I learnt

use filter to reduce the set of similar items instead of explicit if condition
use cached variables i.e. var $this = $(this);
use methods chaining
and if same methods are to be used multiple times, try using it like:

.prop({ disabled: true, title: 'Readonly.' });

instead of call .prop (or any other) multiple times.
try to scope restrict the context of your selector as far as you can, i.e.

jQuery("tr[data-uid='" + v.uid + "'] .k-button", e.sender.wrapper)

so here's my final query:

jQuery(e.sender.dataSource._data).each(function (i, v) {
   if (v.IsReadonly) {
      jQuery("tr[data-uid='" + v.uid + "'] .k-button", e.sender.wrapper)
       .filter(function (i) {
          var $this = jQuery(this);
          return !($this.hasClass('k-grid-edit') || $this.hasClass('k-grid-Desc'));
        })
        .addClass('k-state-disabled')
        .prop({ disabled: true, title: 'Readonly.' });
   }
});

and this query took 267ms - 296ms (for 10 iterations)

-- I hope there can still be something even more faster like getting rid of that parent .each loop


PHP: multilingual Time::since() static class I didn't make the math myself since I'm an idiot. I did however try to make it more versatile and usable for my multilingual page, which involves making it simple to use for advanced plural forms such as Polish and Russian.

Is this code any good to use on a website? or is it overkill?
I'm using the laravel framework, but this code should work everywhere.

I use it like this :

Time::since($user->updated_at)

And here is the code:

class Time
{
public static function since($timestamp) {
    date_default_timezone_set('Europe/Rome');
    $time = strtotime($timestamp);
    $time = time() - $time;

    $tokens = array (
    31536000 => 'year',
    2592000 => 'month',
    604800 => 'week',
    86400 => 'day',
    3600 => 'hour',
    60 => 'minute',
    1 => 'second'
    );

    foreach ($tokens as $unit => $text) {
        if ($time < $unit) continue;
        $numberOfUnits = floor($time / $unit);
        switch ($text)
        {
        case 'year':
        return sprintf(ngettext(
            "Last visit: %d year ago",
            "Last visit: %d years ago",
            $numberOfUnits), $numberOfUnits);
            break;
        case 'month':
            return sprintf(ngettext(
            "Last visit: %d month ago",
            "Last visit: %d months ago",
            $numberOfUnits), $numberOfUnits);
            break;
        case 'week':
            return sprintf(ngettext(
            "Last visit: %d week ago",
            "Last visit: %d weeks ago",
            $numberOfUnits), $numberOfUnits);
            break;
        case 'day':
            return sprintf(ngettext(
            "Last visit: %d day ago",
            "Last visit: %d days ago",
            $numberOfUnits), $numberOfUnits);
            break;
        case 'hour':
            return sprintf(ngettext(
            "Last visit: %d hour ago",
            "Last visit: %d hours ago",
            $numberOfUnits), $numberOfUnits);
            break;
            case 'minute':
            return sprintf(ngettext(
            "Last visit: %d minute ago",
            "Last visit: %d minutes ago",
            $numberOfUnits), $numberOfUnits);
            break;
            case 'second':
            return sprintf(ngettext(
            "Last visit: %d second ago",
            "Last visit: %d seconds ago",
            $numberOfUnits), $numberOfUnits);
            break;
            }
        }
    }
}

I've not coded any PHP in last five years but if I'm right the following is the same:

$tokens = array (
    31536000 => array('msgid1' => "Last visit: %d year ago",
        'msgid2' => "Last visit: %d years ago"),
    2592000 => array('msgid1' => "Last visit: %d month ago",
        'msgid2' => "Last visit: %d months ago"),
    604800 => array('msgid1' => "Last visit: %d week ago",
        'msgid2' => "Last visit: %d weeks ago"),
    86400 => array('msgid1' => "Last visit: %d day ago",
        'msgid2' => "Last visit: %d days ago"),
    3600 => array('msgid1' => "Last visit: %d hour ago",
        'msgid2' => "Last visit: %d hours ago"),
    60 => array('msgid1' => "Last visit: %d minute ago",
        'msgid2' => "Last visit: %d minutes ago"),
    1 => array('msgid1' => "Last visit: %d second ago",
        'msgid2' => "Last visit: %d seconds ago")
);

foreach ($tokens as $unit => $data) {
    if ($time < $unit) {
        continue;
    }
    $numberOfUnits = floor($time / $unit);

    $formatString = ngettext($data['msgid1'], $data['msgid2'], $numberOfUnits);
    return sprintf($formatString, $numberOfUnits);
}

I think you should handle 0 too. Currently it prints nothing for this input.


Simple Logger in C++ I'm currently making a game using C++. I eventually needed a logger, but I wasn't satisfied with any of the existing solutions. After a while, I wrote my own header called log.h:

#include <iostream>
#include <iomanip>
#include <string>
#include <ctime>
#include <fstream>
#include <sstream>

class Logger {
    private:
        std::string logFileName = "log.txt";
        bool print = true;
        bool save = true;

        void log(std::string s, std::string i) {
            time_t cuT;
            struct tm *loT;
            time(&cuT);
            loT = localtime(&cuT);

            std::stringstream ss;
            ss << std::setw(2) << std::setfill('0') << loT->tm_hour << ':';
            ss << std::setw(2) << std::setfill('0') << loT->tm_min << ':';
            ss << std::setw(2) << std::setfill('0') << loT->tm_sec;
            ss << i << s << "\n";

            if(save) {
                std::ofstream of;
                of.open(logFileName, std::ios_base::app | std::ios::out);
                of << ss.rdbuf();
            }
            if(print) {
                std::cout << ss.str();
            }
        }
    public:
        void configure(std::string logFileName_, bool print_, bool save_) {
            this->print = print_;
            this->save = save_;
            this->logFileName = logFileName_;
        }

        void note(std::string s) {
            log(s, " NOTE: ");
        }

        void error(std::string s) {
            log(s, " ERROR: ");
        }

        void warn(std::string s) {
            log(s, " WARNING: ");
        }
};

My library can print the current time as well as other information to the console and save this data into a file. But I did a couple of benchmarks and it turns out that my logging program is a lot slower compared to the normal std::cout.
Since speed is crucial for games, I wanted to ask whether there are ways to optimize my program or, if necessary, to improve it.

Some things to note;

When doing time related things in C++, use std::chrono, not clock().
Your log will not work if it's used in multiple files. Instead of being in a .h file, put the definition of the logger in a cpp file and build both together.
Use include guards for header files

    #ifndef LOG_H
    #define LOG_H

    // Code

    #endif

Define constructors and use them properly and get rid of configure.

    Log(std::string fileName, bool printToConsole, bool saveToFile) :
        logFileName(fileName),
        print(printToConsole),
        save(saveToFile)
    {
    }

Do not use underscores for naming things. General rules for c++ are class names are PascalCase and functions are camelCase.
std::cout is slow, consider using sprintf to make it faster.
Don't write to a file every time you call log. Opening files and writing to them is slow. Consider writing to the file on a timestamp or aggregating a few logs before writing them.


Decompressing a string We are given a string which is compressed and we have to decompress it.

  (a(bc2)d2)$

"$" indicates end of string.

  (abcbcd2)$

  abcbcdabcbcd  (This is the final uncompressed string.)

My code:

import java.util.*;

class decompress
{
    public static void main(String[] ar)
    {
        int end = 0, st = 0;
        Scanner sc = new Scanner(System.in);
        System.out.print(">> ");
        String s = sc.next();
        while(s.indexOf('(') != -1)
        {
        for(int i = 0; i < s.length(); i++)
        {
            char c = s.charAt(i);
            if(c == '(')
                st = i;
            else if(c == ')')
            {
                end = i;
                String t = s.substring(st+1,end);
                int n = t.charAt(t.length()-1) - '0';
                t = t.substring(0,t.length() - 1);
                String q = "";
                for(int j = 0; j < n; j++)
                    q += t;
                String m = "";
                if(st != 0)
                    m = s.substring(0,st);
                s = m + q + s.substring(i+1);
                break;
            }
        }
        }
        System.out.println(s);
     }
}

Is there a more elegant way to make this program ?

Indentation. If you are using Eclipse, please select all your code and press Ctrl + I. Your entire for-loop should be indented one step further.
Use methods. One method for inputting a String, one method for making one iteration of the "decompression".
Did you forget about how to properly use indexOf suddenly? You use indexOf as a condition for your while loop, that makes sense. But then you're using a for-loop to determine the location of the ( and ) characters you are going to work with. This makes no sense to me.
Better variable names. Thanks to your comments on your question, I managed to figure out parts of what you are doing and write a review about that. However, Daniel still has a point that your code is not readable.
Close the scanner. Your scanner is a resource that needs to be closed, call sc.close(); as soon as you are done with the scanner (that is, after you have received all the required inputs from it)
Potential problems. What if I want to encrypt the string (thisWillMessWithYourCode2), how should that string be encrypted in order to be decrypted correctly? Or what would happen if I gave your program the input 42)invalid(data? I believe your code would have a hard time with that.


Working with Exceptions Exercise The task:

There is an AccountManager that makes transfers. It receives 2 arrays of the same size: accounts and sums. They need to be transferred to the accounts correspondingly.

public interface AccountManager {
    public boolean transfer(Account[] accounts, int[] deltas);
}

There is also an Account interface:

public interface Account {
    public void change(int delta) throws TryAgainException, BlockAccountException;
}

If change() throws TryAgainException, we just need to repeat the transfer operation. If change() throws BlockAccountException, we need to roll back the transfer operation. That is, reverse all changes to previous accounts.

AccountClass

public class AccountClass implements Account {

    // setting initial value to 100. so that account would not be empty
    private int accountNum;
    private int currentAccount = 100;

    public AccountClass(int accountNum) {
        this.accountNum = accountNum;
    }

    @Override
    public void change(int delta) throws TryAgainException,
            BlockAccountException {
        System.out.println("account before change" + toString());

        int newBalance = getCurrentAccount() + delta;
        if (newBalance < 0) {
            throw new BlockAccountException(
                    "money cannot be withdrawn as balance will be negative");
        } else if (delta == 100) {
            throw new TryAgainException();
        }
        setCurrentAccount(getCurrentAccount() + delta);

        System.out.println("account after change" + toString());
    }

    public int getCurrentAccount() {
        return currentAccount;
    }

    @Override
    public String toString() {
        return "Account" + accountNum + " [currentAccount=" + currentAccount + "]";
    }

    // making setter private so it will be impossible to modify account from
    // outside
    private void setCurrentAccount(int currentAccount) {
        this.currentAccount = currentAccount;
    }

AccountManagerClass

public class AccountManagerClass implements AccountManager {

    @Override
    public boolean transfer(Account[] accounts, int[] deltas) {
        for (int i = 0; i < accounts.length; i++) {
            int tryNum = 0;
            while (true) {
                try {
                    accounts[i].change(deltas[i]);
                    break;
                } catch (TryAgainException e) {
                    tryNum++;
                    if (tryNum == 5) {
                        break;
                    }
                    System.out.println("trying again");
                } catch (BlockAccountException e) {
                    // rollback
                    System.out.println(e.getMessage());
                    System.out.println("rolling back");
                    if (i > 0) {
                        for (int j = i - 1; j >= 0; j--) {
                            try {
                                accounts[j].change(-deltas[j]);
                            } catch (TryAgainException e1) {
                                // TODO Auto-generated catch block
                                e1.printStackTrace();
                            } catch (BlockAccountException e1) {
                                // TODO Auto-generated catch block
                                e1.printStackTrace();
                            }
                        }
                    }
                    return false;
                }
            }
        }

        return true;
    }

Main

public class Main {

    public static void main(String[] args) {

        AccountManagerClass accountManagerClass = new AccountManagerClass();
        Account[] accounts = {new AccountClass(1), new AccountClass(2), new AccountClass(3), new AccountClass(4)};
        int[] deltas = {100, 200, -200, 99};
        accountManagerClass.transfer(accounts, deltas);
    }

}

// making setter private so it will be impossible to modify account from
// outside
private void setCurrentAccount(int currentAccount) {
    this.currentAccount = currentAccount;
}

So, what's the point of this setter at all? :)


Add/remove class event handler I wrote this if statement event listener to add and remove a class to a couple links based on changed.owl.carousel. However, I know this is not the most DRY method in writing the code and often find myself writing more than is needed. Is there a cleaner way to write this code?

owl.on('changed.owl.carousel', function(event) {
  if ($('.owl-item.active>.item.slide3').length>0) {
    $('#link3').addClass('hover');
  } else if (jQuery('.owl-item.active>.item.slide4').length>0) {
    $('.link').removeClass('hover');
    $('#link4').addClass('hover');
  } else {
    $('.link').removeClass('hover');
  }
})

if ($('.owl-item.active>.item.slide3').length>0) {
  $('#link3').addClass('hover');
} else if (jQuery('.owl-item.active>.item.slide4').length>0) {
  $('.link').removeClass('hover');
  $('#link4').addClass('hover');
} else {
  $('.link').removeClass('hover');
}

I notice the use of both $ and jQuery. Suggesting you use one or the other, but not both. While both are perfectly valid, it's best to keep it consistent.

Also, put a space on both sides of the > for readability. It's not immediately clear that there's actually 2 classes in the selector.

Comparing against 0 is redundant as the number can already double as a boolean. length is a number that's either 0 or a positive integer. Any non-zero number is truthy while 0 is falsy.

I would suggest you avoid IDs. IDs should be unique but nothing prevents you or other code from reusing the same ID. This will lead to problems. For instance, $('#selector') will only return the first of many elements with the same ID.

Without the accompanying HTML, we cannot make assumptions as to how the compare logic works. This is as far as the review goes.


Converting regular text phrase to Pig Latin in Groovy Am I using the best conventions or are there better ways?

    static void makePigLatin(String text){

    String[] textArray = text.split(" ")
    String buffer, translated

    for (int i = 0; i < textArray.length; i++) {

        buffer = ""
        translated = ""

        for (int j = 0; j < textArray[i].length(); j++){

            if (textArray[i][j].find { a -> vowels.any {a.contains(it)}}) {

                translated = textArray[i].substring(j)
                break

            } else {

                buffer = "$buffer${textArray[i][j]}"
            }
        }

        if (textArray[i][textArray[i].length() - 1].find { a -> punctuation.any {a.contains(it)}}) {

            print "${translated.substring(0, translated.length() - 2)}$buffer" + "ay" + "${textArray[i][textArray[i].length() - 1]}" + " "
        } else {

            print "$translated$buffer" + "ay "
        }
    }

    println ""
}

The vowels and punctuation variables are global:

static def vowels = ["a", "e", "i", "o", "u", "y"]
static def punctuation = [",", ".", "?", "!", ":"]

General

Strings

In Groovy you can use single quotes for "regular" strings. Documentation

buffer = ""
// equivalent to
buffer = ''

It's unlike Java where single quotes are for characters.

One exception to this is when using the $variable in Strings, you have to use double quotes for this.

buffer = "$buffer${textArray[i][j]}"
// not equivalent to
buffer = '$buffer${textArray[i][j]}'

Also, when placing variables in Strings, I tend to always wrap in { } even when it's not needed.

buffer = "${buffer}${textArray[i][j]}"
// it's just more explicit

Method signature

This may be an unpopular opinion, but I like not using def in method signatures and instead using explicit types. It makes IDE suggestions and readability a lot better.

static void makePigLatin(String text){
// better than
static void makePigLatin(def text){

To me def is only okay inside of a method.

eachWithIndex

I actually just looked this up writing the answer. You can use a method called eachWithIndex on arrays in place of a closure. Documentation

for (int i = 0; i < textArray.length; i++) {
// or...
textArray.eachWithIndex { e, i ->
    // where i is the index and e is the textArray[i]
}

Scope

buffer and `translated are only ever used in your for loop. It's important to limit the scope of variables as much as possible. So it's probably better to declare them in the loop like so

// String buffer, translated
// remove above

for (int i = 0; i < textArray.length; i++) {

    String buffer = ""
    String translated = ""

Return a String instead of printing

Instead of your method being void make it return a String and remove all the println from it. Then from where you call makePigLatin instead do println makePigLating('some string here').

This limits the side-effects of the method. For tiny programs this is not a big deal but in larger projects knowing exactly what a method will do is very handy -- especially if the method is what is known as a "pure function" which means it literally has no side effects and its output is directly dependent on its output.

Consistent formatting

In some places you have spaces before braces (for (int i = 0; i < textArray.length; i++) {) and in others you don't (static void makePigLatin(String text){). It's less important whether or not you use them than how important it is to be consistent. Personally I put spaces before them.

Specifics

The below code is a little odd to me, it's saying in the ith word in the jth character, find each a in the jth character that is any of the vowels.

textArray[i][j].find { a -> vowels.any {a.contains(it)}}

No need to do a find on a single character. The below is a cleaner way to me.

vowels.any { vowel -> vowel.equals(textArray[i][j]) }

The below

buffer = "$buffer${textArray[i][j]}"

Can simply be

buffer += textArray[i][j]


Vector comparison inside for-loop The following code finds no-rain periods in a time series that were preceded by rain over a threshold (e.g. 10mm in 2 days [k=48]).

significant_drydowns <- function(rain, threshold=10, k=48) {
    require(zoo)
    require(dplyr)

    # Find drydowns: sequences of 0 rain; give them a group
    starts <- (lag(rain, default=0) > 0) & (rain == 0)
    groups <- cumsum(starts)
    groups[rain > 0] <- NaN
    groups[groups == 0] <- NaN

    # remove drydowns where previous rain is below threshold
    past_rain <- rollsum(rain, k, fill=0, align='right')
    for (t in which(starts)) {
        if (past_rain[t-1] < threshold) {
            groups[groups == groups[t]] <- NaN
        }
    }

    return(groups)
}

The for loop is really slow, partly because of the == comparison. Is there any way to speed this code up?

Example data:

rain <-c(0,0,0,2,3,0,0,0,1,5,0,0,0,0,0,0,6,1,1,1,0,0,0)
names(rain) <- rain

Example output:

R> significant_drydowns(rain, 5, 5)
  0   0   0   2   3   0   0   0   1   5   0   0   0   0   0   0   6   1   1   1   0   0   0
NaN NaN NaN NaN NaN   1   1   1 NaN NaN   2   2   2   2   2   2 NaN NaN NaN NaN   3   3   3
R> significant_drydowns(rain, 7, 4)
  0   0   0   2   3   0   0   0   1   5   0   0   0   0   0   0   6   1   1   1   0   0   0
 NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN   3   3   3

So, the group names don't matter, as long as they are unique. Groups are only assigned to dry-downs for which the previous k steps has a sum greater than threshold.

The following function works without for loops or any function of the *apply family. Furthermore, it does not require additional packages but makes use of base functions only. See the code comments for further details.

significant_drydowns <- function(rain, threshold = 10, k = 48) {
  # all values except the first one
  rain_tail <- rain[-1L]
  # logical index of start of no-rain period
  starts <- head(rain, -1L) > 0 & !rain_tail
  # no-rain groups
  groups <- cumsum(starts)
  # sum of the amount of rain in the last k elements
  # (for e.g., k = 5 the filter is c(1,1,1,1,1), therefore
  # the sum of 5 preceding elements is calculated)
  past_rain <- filter(rain, rep.int(1L, k), sides = 1L)
  # valid groups (previous amount of rain exceeds threshold)
  valid <- past_rain[starts] >= threshold
  # replace `groups` values with NA if
  #  (a) there is rain or
  #  (b) this is the first rain or no-rain period
  is.na(groups) <- rain_tail | !groups
  # replace `groups` values with NA if amount of rain is below threshold
  # (here `groups` is used as a numeric index for `valid`)
  is.na(groups) <- !valid[groups]
  # add NA to match length of original vector and set names
  setNames(c(NA_integer_, groups), rain)
}

Some results:

> significant_drydowns(rain, 5, 5)
 0  0  0  2  3  0  0  0  1  5  0  0  0  0  0  0  6  1  1  1  0  0  0
NA NA NA NA NA  1  1  1 NA NA  2  2  2  2  2  2 NA NA NA NA  3  3  3
> significant_drydowns(rain, 7, 4)
 0  0  0  2  3  0  0  0  1  5  0  0  0  0  0  0  6  1  1  1  0  0  0
NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA  3  3  3


Acquiring payment service ID I have a utility class, called Bills, that extracts some information from a billId and a paymentId. I'm pasting the whole class here for completeness, you don't need to understand the algorithm behind all functions to be able to answer my question.

<?php

class Bills {
    private $typeAndServicePaymentIdMap = [
        0 => 13,
        1 => 4, // water
        2 => 1, // electricity
        3 => 5, // gas
        4 => 2, // landline
        5 => 3, // cellphone
        6 => 6, // municipality due
        7 => 6, // municipality due type 7
        9 => [
            'default' => 24, // traffic due
            '001' => 38, // traffic fine
            '002' => 38, // traffic fine
        ],
    ];

    private $logger;

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

    public function getType($billId)
    {
        return substr($billId, -2, 1);
    }

    public function getAmount($paymentId)
    {
        $paymentId = str_pad($paymentId, 13, '0', STR_PAD_LEFT);
        $amount = ltrim(substr($paymentId, 0, 8), '0') . '000';

        return $amount;
    }

    public function getCompanyCode($billId)
    {
        return substr($billId, -5, 3);
    }

    public function getPaymentServiceId($billId)
    {
        $type = $this->getType($billId);

        if (key_exists($type, $this->typeAndPaymentServiceIdMap)) {
            return $this->calculatePaymentServiceId($type);
        } else {
            $this->logger->errorLog('Type ' . $type . ' is not defined in the $typeAndPaymentServiceIdMap.');
        }
    }

    private function calculatePaymentServiceId($type)
    {
        $paymentServiceId = $this->typeAndPaymentServiceIdMap[$type];

        if (is_array($paymentServiceId)) {
            return $this->createPaymentServiceIdFromCompanyCode($paymentServiceId);
        } else {
            return $paymentServiceId;
        }
    }

    private function createPaymentServiceIdFromCompanyCode($paymentServiceId)
    {
        $companyCode = $this->getCompanyCode($billId); // TODO: fix this, we don't have $billId here

        if (key_exists($companyCode, $paymentServiceId)) {
            return $paymentServiceId[$companyCode];
        } else {
            return $paymentServiceId['default'];
        }
    }
}

The problem I have is in getPaymentServiceId(). Here is what this function does:

If the type is found in the type -> paymentServiceId map, it checks the value defined for $type in the map. For example, if $type is 1, it gets the value for 1, which in this case is 4.
If the paymentServiceId (the value that was retrieved in the previous step) is not an array, it just returns it. For example, if type is 1, the value is 4, and it is returned as-is.
However, if the paymentServiceId is an array, we need to calculate the company code (calculated using $this->getCompanyCode()) to be able to get a paymentServiceId, and as you can see, to do that, we need billId. In the current mapping, only a type of 9 would result in this calculation. All other types would not require this.

In other words, if createPaymentServiceIdFromCompanyCode() is called, it needs billId. If not, billId is not needed. My problem is that I don't want to pass billId as an argument down the chain when it is only needed for one $type.

Is there a better way of structuring this class to achieve what I want, or do I have to just pass billId down the chain every time?

If you want to avoid passing $billId argument down the chain within Bills class - "stub" chain of those methods.
Let the getPaymentServiceId() method do all the "processing" work. Remove calculatePaymentServiceId and createPaymentServiceIdFromCompanyCode methods.  The final result should look like the following:

class Bills {

    private $typeAndPaymentServiceIdMap = [
        0 => 13,
        1 => 4, // water
        2 => 1, // electricity
        3 => 5, // gas
        4 => 2, // landline
        5 => 3, // cellphone
        6 => 6, // municipality due
        7 => 6, // municipality due type 7
        9 => [
            'default' => 24, // traffic due
            '001' => 38, // traffic fine
            '002' => 38, // traffic fine
        ],
    ];
    private $logger;

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

    public function getType($billId = "") {
        return substr($billId, -2, 1);
    }

    public function getAmount($paymentId = "") {
        $paymentId = str_pad($paymentId, 13, '0', STR_PAD_LEFT);
        $amount = ltrim(substr($paymentId, 0, 8), '0') . '000';

        return $amount;
    }

    public function getCompanyCode($billId = "") {
        return substr($billId, -5, 3);
    }

    public function getPaymentServiceId($billId = "") {
        if (!$billId) {
            $this->logger->errorLog('billId is required!');
            return;
        }
        $type = $this->getType($billId);
        $companyCode = $this->getCompanyCode($billId);

        if (key_exists($type, $this->typeAndPaymentServiceIdMap)) {
            $paymentServiceId = $this->typeAndPaymentServiceIdMap[$type];
            if (is_array($paymentServiceId)) {
                return (key_exists($companyCode, $paymentServiceId))? $paymentServiceId[$companyCode] : $paymentServiceId['default'];
            } else {
                return $paymentServiceId;
            }
        } else {
            $this->logger->errorLog('Type ' . $type . ' is not defined in the $typeAndPaymentServiceIdMap.');
        }
    }
}

// $logger object
$bill = new Bills($logger);

var_dump($bill->getPaymentServiceId("00190"));  // int 38
var_dump($bill->getPaymentServiceId("00390"));  // int 24


Advent of Code 2019 Day 6 - Beginner Rust solution I am doing Advent of Code 2019 in order to learn Rust (it's been fun, but challenging!).

I am looking for help and feedback from more experiences Rustaceans on my solution to Day 6: Universal Orbit Map in Rust.

Specifically I am looking to see where I am not doing it the idiomatic Rust way, where my Java background shines through or anything that doesn't look or feel right or Rust-ish.

use std::collections::HashMap;
use std::io;
use std::io::Read;

fn main() {
    let mut input = String::new();
    match io::stdin().read_to_string(&mut input) {
        Ok(v) => v,
        Err(_) => panic!("Unable to read input"),
    };
    let mut orbits = HashMap::new();
    input
        .lines()
        .map(|l| l.split(")").collect::<Vec<&str>>())
        .for_each(|o| {
            orbits.insert(o[1].to_string(), o[0].to_string());
        });
    let num_orbits = orbits
        .iter()
        .map(|k| path(k.0, &orbits))
        .map(|p| p.len())
        .sum::<usize>();

    let mut you_path = path("YOU", &orbits);
    let mut san_path = path("SAN", &orbits);

    while you_path.get(you_path.len() - 1) == san_path.get(san_path.len() - 1) {
        you_path.pop();
        san_path.pop();
    }

    println!(
        "Number of orbits: {}, Number of jumps: {}",
        num_orbits,
        (you_path.len() + san_path.len())
    );
}

fn path(from: &str, orbits: &HashMap<String, String>) -> Vec<String> {
    let mut p = Vec::new();
    let mut inner = from;
    loop {
        match orbits.get(inner) {
            Some(v) => {
                p.push(v.to_string());
                inner = v;
            }
            None => break,
        };
    }
    p
}

Disclaimer: I don't know Rust.

I only have comments on performance.

To compute the number of orbits, it's unnecessary to compute paths, you could compute just the length of paths. That would reduce the space complexity to constant.

When computing the number of orbits, instead of going from each planet until the root, you could go from the root, traversing paths to every planet exactly once. That would reduce the time complexity to linear.

When computing the common ancestor from SAN and YOU, you only need to track part of the path segments, until a common point.


Edge detection in a canvas I have one function in my code to detect edges in a canvas but it's the slowest part of my code and is used everywhere. If I could just optimise this, my whole program would speed up considerably.

Maybe something better can be done with Em6?

Basically the function goes from the left until it finds a pixel colour greater (or less than) the colour value passed to the function. Then it gets the maximum or minimum of all the values which is the edge of whatever is inside the image.

Then it does the same for the top, the right the bottom to find all edges. Useful for cropping something within a canvas.

function getEdges(tempcanv, colour, which, condition) {
    let ledge = [], redge = [], tedge = [], bedge = [];
    const canvwidth = tempcanv.width, canvheight = tempcanv.height;
    const contextd = tempcanv.getContext('2d');
    for (let y = 0; y < canvheight; y++) {//left edge
        for (let x = 0; x < canvwidth; x++) {
            const data = contextd.getImageData(x, y, 1, 1).data;
            if (condition(data[which], colour)) {
                ledge.push(x);
                break;
            }
        }
    }
    ledge = Math.min(...ledge);
    const llim = ledge === 0 ? ledge : ledge - 1;
    for (let x = llim; x < canvwidth; x++) {//top edge
        for (let y = 0; y < canvheight; y++) {
            const data = contextd.getImageData(x, y, 1, 1).data;
            if (condition(data[which], colour)) {
                tedge.push(y);
                break;
            }
        }
    }
    tedge = Math.min(...tedge);
    const tlim = tedge === 0 ? tedge : tedge - 1;
    for (let y = tlim; y < canvheight; y++) {//right edge
        for (let x = canvwidth - 1; x >= ledge; x--) {
            const data = contextd.getImageData(x, y, 1, 1).data;
            if (condition(data[which], colour)) {
                redge.push(x);
                break;
            }
        }
    }
    redge = Math.max(...redge);
    for (let x = llim; x <= redge; x++) {//bottom edge
        for (let y = canvheight - 1; y >= tedge; y--) {
            const data = contextd.getImageData(x, y, 1, 1).data;
            if (condition(data[which], colour)) {
                bedge.push(y);
                break;
            }
        }
    }
    return [ledge, tedge, redge + 1, Math.max(...bedge) + 1]
}

//(canvas passed, colour threshold, R,G or B, greater or less than)
getEdges(canvas, 180, 0, (a, b) => a > b))

My code does have some optimisation since as it finds an edge, the next edge check does not check the dead space already checked.

Here is a test shape.

In your code you're calling GetImageData for each pixel adding a lot of call that are not necessary, you can do this by calling it once for the entire canvas and getting the data from the array.

GetImageData returns a 1 dim array with RGBA for each pixel like :

[R(pixel1),G(Pixel1),B(Pixel1),A(Pixel1),R(pixel2),G(Pixel2),...etc]

this means that you can get the pixel value by using (y * canvasWidth+ x)*4+RGBA
(RBGA is a 0-3 value representing the 4 values)

Another thing you can improve on is the order of your for loops. The way you're looping now you need to loop through the whole image before you can know the edge, however if you turn the loops around you can lessen the amount of pixels you need to check.

For example when going for the left edge you can start the double loop with x then y. This means that whenever the condition first becomes true you can be sure that it is the outer most left edge.

If you implement these relative easy changes the code would look more like this:

function getEdges(tempcanv, colour, which, condition) {
    let ledge = -1, redge = -1, tedge = -1, bedge = -1;
    const canvwidth = tempcanv.width, canvheight = tempcanv.height;
    const imageData = tempcanv.getContext('2d').getImageData(0, 0, canvwidth, canvheight).data; //gets all image data
    for (let x = 0; x < canvwidth; x++) {//left edge
        if (ledge >= 0) {
            break;
        }
        for (let y = 0; y < canvheight; y++) {
            //(y * canvwidth + x) * 4 is for getting the current location
            if (condition(imageData[(y * canvwidth + x) * 4 + which], colour)) {
                ledge = x;
                break;
            }
        }
    }
    for (let x = canvwidth - 1; x >= ledge; x--) {//right edge
        if (redge >= 0) {
            break;
        }
        for (let y = 0; y < canvheight; y++) {
            //(y * canvwidth + x) * 4 is for getting the current location
            if (condition(imageData[(y * canvwidth + x) * 4 + which], colour)) {
                redge = x;
                break;
            }
        }
    }
    for (let y = 0; y < canvheight; y++) {//top edge
        if (tedge >= 0) {
            break;
        }
        for (let x = ledge; x <= redge; x++) {
            //(y * canvwidth + x) * 4 is for getting the current location
            if (condition(imageData[(y * canvwidth + x) * 4 + which], colour)) {
                tedge = y;
                break;
            }
        }
    }
    for (let y = canvheight-1; y >= tedge; y--) {//bottom edge
        if (bedge >= 0) {
            break;
        }
        for (let x = ledge; x <= redge; x++) {
            //(y * canvwidth + x) * 4 is for getting the current location
            if (condition(imageData[(y * canvwidth + x) * 4 + which], colour)) {
                bedge = y;
                break;
            }
        }
    }
    console.log('edges', [ledge, tedge, redge + 1, bedge + 1]);
    return [ledge, tedge, redge + 1, bedge + 1]
}


Looping text in Python with directional control The code is an implementation of looping text (similar to a circular buffer - wraps around when it reaches the edge of the defined bounds) with directional control.

The code is functional and works as intended, but I'm curious if there is anyway to improve the code even if it is only a small improvement (e.g. less garbage produced per loop cycle as a micro-optimisation, etc).

# cleaned up and improved prototype code for looping text scroll.
# added direction from the original code.

from sys import stdout
from time import sleep

global_shifted_text_array = []
global_text_length = 0

DIRECTION_LEFT_TO_RIGHT = -1
DIRECTION_RIGHT_TO_LEFT = 1

def set_text(text):
    global global_shifted_text_array
    global global_text_length

    global_shifted_text_array = pad_text(text)
    global_text_length = len(global_shifted_text_array)

def pad_text(text, padding = 5):
    text += ' ' * padding
    return list(text)

def shift_text(direction):
    global global_shifted_text_array
    range = xrange(global_text_length - 1, -1, -1) if direction == DIRECTION_LEFT_TO_RIGHT else xrange(global_text_length)

    # by the time the zero element is set in the for loop.
    # it is set to the last element (which is empty)
    # so we have to account for this by correcting for it.
    if direction == DIRECTION_RIGHT_TO_LEFT:
        first_element = global_shifted_text_array[0]

    for i in range:
        global_shifted_text_array[i] = global_shifted_text_array[((i + direction) % global_text_length)]
        # print 'global_shifted_text_array[{}] = global_shifted_text_array[{}]'.format(i, ((i + direction) % global_text_length))

    if direction == DIRECTION_RIGHT_TO_LEFT:
        global_shifted_text_array[global_text_length - 1] = first_element

def update_loop(direction = DIRECTION_LEFT_TO_RIGHT, frequency = 0.1):
    while 1:
        shift_text(direction)
        stdout.write('\r{0}'.format(''.join(global_shifted_text_array)))
        sleep(frequency)

set_text('Beautiful Lie')
update_loop()

When I try it, it doesn't really work unless I add stdout.flush() after each stdout.write().

The padding isn't controllable, as neither set_text() nor update_loop() accepts a padding parameter.

In my opinion, it should print the original text before performing the first shift.

Global variables are bad.  In this case, I'd eliminate them altogether by simplifying the code.  Take advantage of itertools.cycle() and Python's support for negative indexes.  I wouldn't bother with converting the string to a list and back to a string again.

DIRECTION_LEFT_TO_RIGHT is a bit verbose.  I suggest SCROLL_RIGHT instead.  "Frequency" sounds like it should go faster as the number increases: 10 Hz is faster than 1 Hz.  I suggest period as the name for the delay.

from itertools import cycle
from sys import stdout
from time import sleep

SCROLL_RIGHT = -1
SCROLL_LEFT = +1

def animate_text(text, scroll=SCROLL_RIGHT, period=0.5, padding=5):
    text += ' ' * padding
    for offset in cycle(xrange(0, scroll * len(text), scroll)):
        stdout.write('\r' + text[offset:] + text[:offset])
        stdout.flush()
        sleep(period)

animate_text('Beautiful Lie')


Optimizing insertion sort for small amounts of data I'm building a hybrid sort, and for that I need a fast and adaptive sort ideal for small sizes (< 65 elements).

Insertion sort immediately comes to mind, and I've been tinkering with different implementations of it. My requirement is that it takes in iterators as per C++ standard.

Linear insertion sort

template <typename Iter>
void lin_sort(Iter begin, Iter end) {
    for (auto cur = begin; cur != end; ++cur) {
        auto key = *cur;
        auto ins = cur - 1;
        for (; begin <= ins && key < *ins; --ins)
            *(ins + 1) = *ins;    // move 1 to right until correct position's found
        *(ins + 1) = key;
    }
}

Binary insertion sort

template <typename Iter>
void ins_sort(Iter begin, Iter end) {
    for (auto cur = begin; cur != end; ++cur) {
        // upper_bound is binary search for correct insertion position
        // shift everything from up to cur to the right one and insert key into right place
        std::rotate(std::upper_bound(begin, cur, *cur), cur, cur + 1);
    }
}

Alternative and equivalent binary insertion sort

template <typename Iter>
void ins_sort(Iter begin, Iter end) {
    for (auto cur = begin; cur != end; ++cur) {
        auto key = *cur;
        auto ins = std::upper_bound(begin, cur, key);
        for (auto shift = cur; shift != ins; --shift) *shift = *(shift-1);
        *ins = key;
    }
}

Some benchmarking reveals the binary version to be around 1.5 times slower than the linear version, compiling on GCC 4.8.2 with -O3

mylibs\algo>algotest s l
after: 100000 lists with 64 elements: 1.11421e+06 us    1114.21 ms  (linear insertion sort)
mylibs\algo>algotest s i
after: 100000 lists with 64 elements: 1.62741e+06 us    1627.41 ms  (binary insertion sort)

I am only concerned about its performance for under 65 elements, so the asymptotic guarantee given by binary search is irrelevant. Can anyone suggest improvements to the linear insertion sort, or a better alternative for what I'm looking for?

Benchmarking code: (full testing can be found in algotest)
Generating random data

// random number generation
class Rand_int {
public:
    Rand_int(int low, int high) : distribution{low, high} {
    }
    int operator()() {
        return distribution(engine);
    }

private:
    default_random_engine engine;
    uniform_int_distribution<> distribution;
};

vector<int> randgen(int max, int num) {
    static Rand_int die{0, max};
    vector<int> res;
    res.reserve(num);
    for (int i = 0; i < num; ++i)
        res.push_back(die());
    return res;
}

template <typename T>
vector<vector<T>> data_gen(const string& fname, int l_num = list_num, int l_size = list_size, int r = range) {
    ofstream f {fname};
    f << l_num << ' ' << l_size << ' ' << r << endl;
    vector<vector<T>> v_list;
    v_list.reserve(l_num);
    for (int i = 0; i < l_num; ++i) v_list.push_back(randgen(r, l_size));
    for (auto v : v_list) print(v, f);
    return v_list;
}

template <typename Container>
void print(const Container& v, ostream& os = cout) {
    for (auto x : v)
        os << x << ' ';
    os << '\n';
}

Timing

class Timer {
    time_point<system_clock> init, end;
    microseconds elapsed = duration_cast<microseconds>(end - init);
public:
    Timer() : init{system_clock::now()} {}
    void start() { init = system_clock::now(); }
    double tonow() {
        end = system_clock::now();
        elapsed = duration_cast<microseconds>(end - init);
        return elapsed.count();
    }
};

//... inside switch case of algotest main
    load_data(v_list);  // v_list is loaded from file
    switch (argv[2][0]) {
        // ... more sorts
        case 'i': for(auto& v : v_list) ins_sort(v.begin(), v.end()); break;
        case 'l': for(auto& v : v_list) lin_sort(v.begin(), v.end()); break;
        default: cout << "incorrect sort selection\n"; listalgos();
    }

Unfortunately, you are unlikely to find a better sort than the linear insertion sort when it comes to sorting small data sets. The only sorting algorithms that might be faster are sorting algorithms specialized for a given size of input: for example, optimized sorting networks can be really fast if you need to sort a small amount of integers, but are generally slower than the insertion sort if not implemented properly or if used with other types. The following image is a graph that shows the performance of different kinds of sorting algorithms to sort small fixed-size integer arrays (the time in ms corresponds to the time needed to sort one million arrays of a given size). Distributions had a random pattern:

Some of the algorithms were specialized for fixed-size collection; most notably low_comparisons_sorter, low_moves_sorter and sorting_network_sorters. The linear insertion sort was the fastest sorting algorithm which worked for collections of any size; my best bet is that a small collection fits in the fastest cache and thus benefits from the linear access.

Now, the linear version of the insertion sort I use is slightly different: it has a check to make sure that no temporary value is created if the current element is already in place. In your algorithm, it would look like that:

template <typename Iter>
void lin_sort(Iter begin, Iter end) {
    for (auto cur = begin; cur != end; ++cur) {
        auto ins = cur - 1;
        if (*cur < *ins) {
            auto key = *cur;
            do {
                *(ins + 1) = *ins;  // move 1 to right until correct position's found
                --ins;
            } while (begin <= ins && key < *ins);
            *(ins + 1) = key;
        }
    }
}

I doubt it would change a thing, but you could also try to find the point where to insert the value in a linear fashion, then rotate the values. It may be simpler for a compiler to vectorize code that linearly compares things then linearly copies things rather than a code that mixes comparison and copy operations on-the-fly. I did not test that, so don't take it for granted.

To sum up: for small collection, it's unlikely that you will beat linear insertion sort unless you have specializations for some specific types or some specific sizes, which means more code.


Listing all paths through a matrix, stepping right or down only I have the following method, which finds all the possible paths from the top left to bottom right of an N x M matrix. I was wondering what is the best way to optimize it for speed as it is a little slow right now. The resulting paths are then stored in a set.You can only move down or right to an adjacent spot, no diagonals from your current position.

    static public void printPaths (String tempString, int i, int j, int m, int n, char [][] arr, HashSet<String> palindrome) {
        String newString = tempString + arr[i][j];
        if (i == m -1 && j == n-1) {
            palindrome.add(newString);
            return;
        }
        //right
        if (j+1 < n) {
            printPaths (newString, i, j+1, m, n, arr, palindrome);
        }
        //down
        if (i+1 < m) {
            printPaths (newString, i+1, j, m, n, arr, palindrome);
        }
    }

static public void printPaths (String tempString, int i, int j, int m, int n, char [][] arr, HashSet<String> palindrome) {

You say palindrome? Maybe you shouldn't recycle names from other tasks.

This method should not be public. And should not be called "printWhatever" as it - fortunately - does not print.

arr is an array, nice, but then why did you write "matrix" in your text? Maybe, because of matrix being a more descriptive name? So use it.

The arguments m and n are redundant as they can be obtained from the matrix (and it's improbably more time-consuming than passing them around). Too many arguments make a method hard to use and to read.

So the public method could be

static public Set<String> allPaths(char [][] matrix) {
    Set<String> result = new HashSet<>();
    addAllPathsTo(result, matrix, "", 0, 0);
    return result;
}

with a helper like

static private void addPathsTo(Set<String> result, String tempString, int i, int j, char [][] matrix) {
    String newString = tempString + arr[i][j];
    int m = matrix.length;
    int n = matrix[0].length;
    if (i == m-1 && j == n-1) {
        result.add(newString);
        return;
    }
    //right
    if (j+1 < n) {
        addPathsTo(result, newString, i, j+1, matrix);
    }
    //down
    if (i+1 < m) {
        addPathsTo(result, newString, i+1, j, matrix);
    }
}

The optimization by JS1 makes a lot of sense and could be included. My point was clean up only.


User class implemented with PHP 7.4 using Active Record I need some constructive feedback for the working OOPs project which I created to introduce to Object Oriented Programming in a class. Hence is for beginners, I did not include interfaces or abstract functions nor singleton or factories. Can you give me some feedback on the code style, some possible anti patterns? Thanks

<?php

declare(strict_types=1);

namespace MidoriKocak;

use function array_key_exists;

/**
 *
 */
final class User
{

    /**
     * @var Database
     */
    private Database $db;
    /**
     * @var string
     */
    private ?string $id = null;

    /**
     * @var string
     */
    private string $email;
    /**
     * @var string
     */
    private string $username;
    /**
     * @var string
     */
    private string $password;

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

    /**
     * @param  string  $password
     *
     * @return void
     */
    public function setPassword(string $password): void
    {
        $this->password = password_hash($password, PASSWORD_DEFAULT);
    }

    public function getPassword(): string
    {
        return $this->password;
    }

    /**
     * @param  string  $email
     *
     * @return void
     */
    public function setEmail(string $email): void
    {
        $this->email = $email;
    }

    /**
     * @param  string  $username
     *
     * @return void
     */
    public function setUsername(string $username): void
    {
        $this->username = $username;
    }

    /**
     * @return void
     */
    public function save()
    {
        if ($this->id) {
            $this->db->update($this->id, $this->toArray(), 'users');
        } else {
            $this->db->insert($this->toArray(), 'users');
            $this->id = $this->db->lastInsertId();
        }
    }

    /**
     * @return array
     */
    public function toArray(): array
    {
        return [
            'username' => $this->username,
            'email' => $this->email,
            'password' => $this->password
        ];
    }

    /**
     * @param  array  $data
     *
     * @return void
     */
    public function fromArray(array $data): void
    {
        if (array_key_exists('id', $data)) {
            $this->id = (string)$data['id'];
        }

        $this->setEmail($data['email']);
        $this->setUsername($data['username']);
        $this->setPassword($data['password']);
    }
}

Thank you for your feedback.

(P.S. Please be harsh as possible.)

Inconsistent Function Imports

Import all used functions or none.
If I see this:

use function array_key_exists;

I assume that it is the only global function being used in that file. To my surprise, it is not true as password_hash is used inside the same file too.

Either import them all, or none.
I myself prefer no imports and instead prefix global function calls with the backslash to denote global scope (ie return \array_key_exists($k, $a);).

Useless Docblocks

In PHP 7.4 you get the benefit of typehinting properties. I dont see a reason to repeat that type in a docblock anymore in that case. Furthermore it may cause confusion of what the original intent was, if docblock and typehint mismatch. Like here:

/**
 * @var string
 */
private ?string $id = null;

So can it be null or not? I suppose it can, but you never know if you made it this confusing...

Active Record Anti-pattern

You asked to identify possible anti patterns. Active record is one of them. It combines two responsibilities. The entity should not know anything about where it is going to be stored and how. It may eventualy get stored on multiple places or be stored in various ways. The entity should only know that it has some structured data. Then another class(es) should know how to store it and reconstruct it from its permanent representation (ie a db row).

Also notice that save() is either going to have to be repeated in every entity. Or all entities must inherit the same parent or use the same trait (inheritance is oftne not a good idea, and multiple inheritance (traits) even worse).

Also notice that only one of User class's methods (namely the save) uses the database object. That only confirms that it should not be there. You should pass the user entity/structure to another object asking him to save the user into its persistent storage for later retrieval.

Check Your Arrays' Keys

In the fromArray() you are copying the values from array data without making sure that those data is there.

$this->setEmail($data['email']);
$this->setUsername($data['username']);
$this->setPassword($data['password']);

if (isset($data['email']) && \is_string($data['email'])) $this->setEmail($data['email']);

or set it to some default

$this->setEmail($data['email'] ?? '');

Handling Login

Although you have removed that part from your post, I will address it nevertheless.

isLoggedIn should not be persistent property of user entity (table). It should be stored (or inferred) from session. Otherwise how you make sure that it is turend to false after some time of inactivity?

Anyway User::login(string $email, string $pass): bool again breaks SRP. You should have it more like this: Authenticator::login(string $email, string $pass): ?User. You may be tempted to make it UsersRepository::login() but repository usualy does not need access to session while the login method does, therefore it should be in its own class, not in the repository.


Implementation of Scytale cipher (encryption and decryption) This is web exercise 3.1.41. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

The scytale cipher is one of the first cryptographic devices used for
military purposes. (See The Code Book.) It
was used by the Spartans in the fifth century BCE. To scramble the
text, you print out every kth character starting at the beginning,
then every kth character starting at the second character, and so
forth. Write a program implements this encryption scheme.

Here is my program:
public class ScytaleCipher
{
    public static String encrypt(String text, int kth)
    {
        int textLength = text.length();
        String encrypted = "";
        for (int i = 0; i < kth; i++)
        {
            for (int j = 0; i+j < textLength; j += kth)
            {
                encrypted += String.valueOf(text.charAt(i+j));
            }
        }
        return encrypted;
    }
    public static String decrypt(String text, int kth)
    {
        return encrypt(text, kth-1);
    }
    public static void main(String[] args)
    {
        int kth = Integer.parseInt(args[0]);
        String message = "";
        while (!StdIn.isEmpty())
        {
            String word = StdIn.readString();
            message += word;
        }
        // to encrypt
        System.out.println(encrypt(message, kth));
        // to decrypt
        //System.out.println(decrypt(message, kth));
    }
}

StdIn is a simple API written by the authors of the book. I checked my program and it works.
Is there any way that I can improve my program?
Thanks for your attention.

I have some suggestions for your code.
Use java.lang.StringBuilder to concatenate String in a loop.
It's generally more efficient to use the builder in a loop, since the compiler is unable to optimize it by itself while translating your code into bytecode; The compiler will not use the java.lang.StringBuilder in complex loops and your method will take more time and more memory to execute, since the String Object is immutable (a new instance will be created each iteration).
StringBuilder encrypted = new StringBuilder();
for (int i = 0; i < kth; i++) {
   for (int j = 0; i + j < textLength; j += kth) {
      encrypted.append(text.charAt(i + j));
   }
}
return encrypted.toString();

When concatenating a primitive into a string, you don’t need to convert it.
You can add the primitive (int, double, float, char, ect) directly into the string by using the assignment operators (+, +=).
Before
encrypted += String.valueOf(text.charAt(i+j));

After
encrypted += text.charAt(i+j);


Automating project budgets I use this code in a GAS (Google Apps Script) to automate my project budgets.

Since every project has its own spreadsheet but all functions are in the same script file, being called as a library, I can't use them directly. I need to call libraryName.functionName, but it can't serve any arguments unless I do this:

function funcoesOrcamento( funcao, args ){
  if(args)
    args = args.split("\!|");
  else
    args = [];

  switch( args.length ){
    case 0:
      return FuncoesOrcamentoV2[ funcao ]();
    case 1:
      return FuncoesOrcamentoV2[ funcao ]( args[0] );
    case 2:
      return FuncoesOrcamentoV2[ funcao ]( args[0], args[1] );
    case 3:
      return FuncoesOrcamentoV2[ funcao ]( args[0], args[1], args[2] );
    case 4:
      return FuncoesOrcamentoV2[ funcao ]( args[0], args[1], args[2], args[3] );
    case 5:
      return FuncoesOrcamentoV2[ funcao ]( args[0], args[1], args[2], args[3], args[4] );
  }
}

Is there any way to make this more elegant and accept any number of args?

You can use .apply() which takes an array of arguments and those arguments will then be passed to the actual function as a series of normal arguments:

function funcoesOrcamento( funcao, args ){
  if(args)
    args = args.split("\!|");
  else
    args = [];

  return FuncoesOrcamentoV2[ funcao ].apply(this, args);
}

You can read more about .apply() here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply


Who wants a FizzBuzz? I have been playing around with the Io language, and this is my second toy, after the yes program that is just loop ("yes" printl) and on which not much review is possible, I wrote a FizzBuzz program. I like the shortness of it, but any improvement is welcome as I am very beginner in this programming language.

toBuzzWord := method(n,
   if (n % 15 == 0, "FizzBuzz",
   if (n % 5 == 0, "Fizz",
   if (n % 3 == 0, "Buzz", n))))

for (i, 1, 100, toBuzzWord(i) println)

From discussion elsewhere I know you don't want to change the basic implementation, so I'll confine my comments to idiom, style and tweaks.

I would replace the for loop with a foreach

Range // addon reference required in newer versions
1 to(100) foreach(i, toBuzzWord(i) println)

The Range protocol used to be in the core library but is now an add-on, hence the reference needed before you can use it.  That said, it's a more idiomatic, expressive and robust way to deal with simple ranges.  for loops are even more fragile in Io than in C-like languages, given the optional parameter and Io's blithe tolerance of extra parameters.

I don't like chained if-then-elsif in any language, even when it's a function and not a keyword.  One alternative would be to use return

method(n,
   if (n % 15 == 0, return "FizzBuzz")
   if (n % 5 == 0, return "Fizz")
   if (n % 3 == 0, return "Buzz")
   return n
)

Multiple exit points from a function are much debated and criticised, but

Io has no switch or case function and the community doesn't want one.
lists (using a numeric index lookup) or maps (using a string key) are sometimes used for very simple switch-style control flows but that doesn't work here.

In purist OO style, Io developers prefer to use (no surprise here) polymorphism.  Send a message and let the inheritance chain pick the appropriate action.

Polymorphism is really overkill for the simple fizzBuzz implementation you have chosen.  It might be useful for a more efficient solution.


Age in years at a specific date For eligibility to events I need to know the age of a competitor at a specific date.

Now the thing is the date calculations are checked in two places, an online PHP app where the entries are registered, and an offline Java app that runs the events.

So I have 2 versions of the same code, the PHP one was written originally and the Java is a conversion of that. (So that I am using the same calculations in both places).

My Java skills are very rusty.

I am happy to take any improvements onboard, but it is essential that both calculations return the same result.

PHP

public function ageAt($age_reference_date) {
    $ts_ref = strtotime($age_reference_date);
    list($dob_year, $dob_month, $dob_day) = explode('-', $this->date_of_birth);

    $age = date('Y', $ts_ref) - $dob_year;

    if (date('md', $ts_ref) < $dob_month.$dob_day) {
        $age--;
    }

    return $age;
}

Java

public int ageAt(Date ageReferenceDate) {
    Calendar calAgeReferenceDate = Calendar.getInstance();
    Calendar calDateOfBirth = Calendar.getInstance();
    int refDateMonth;
    int dobDateMonth;
    int age;

    calAgeReferenceDate.setTime(ageReferenceDate);

    calDateOfBirth.setTime(this.dateOfBirth);
    age = calAgeReferenceDate.get(Calendar.YEAR) - calDateOfBirth.get(Calendar.YEAR);

    refDateMonth = Integer.parseInt(String.format("%02d", calAgeReferenceDate.get(Calendar.MONTH)) + String.format("%02d", calAgeReferenceDate.get(Calendar.DAY_OF_MONTH)));
    dobDateMonth = Integer.parseInt(String.format("%02d", calDateOfBirth.get(Calendar.MONTH)) + String.format("%02d", calDateOfBirth.get(Calendar.DAY_OF_MONTH)));

    if (refDateMonth < dobDateMonth) {
        age--;
    }

    return age;
}

In Java 8 you can more accurately represent the dateOfBirth with a LocalDate.
If the ageAt() method also takes a LocalDate as argument, then the age is easily computed using Period.

public int ageAt(LocalDate ageReferenceDate) {
    return Period.between(dateOfBirth, ageReferenceDate).getYears();
}

It is fairly easy, albeit a bit verbose, to convert a Date instance to a LocalDate :

LocalDate localDate = ZonedDateTime.ofInstant(date.toInstant(), ZoneId.systemDefault()).toLocalDate();

The catch is that you need to interpret it vs. a ZoneId. A Date can be mapped to different LocalDates depending on what ZoneId (TimeZone's successor) it is evaluated against. You should use the ZoneId that was used when the Date was recorded. That is probably ZoneId.systemDefault(), but if your code can run on systems accross time zones, you can have a problem.

This is also the reason why it is a better idea to represent date of birth as a LocalDate, since it does away with that ambiguity.


Batch Insert Figures Into Powerpoint Slides with VBA I have a series of images ("1.bmp", "2.bmp", "3.bmp",... ,"30.bmp" in "F:\Images" folder) and I am trying to insert these images one by one into Microsoft PowerPoint with VBA program. The base slide (as template) has been set down in page 2. The image object which is named Image1 in that template slide is going to be replaced into "F:\Images\1.bmp", "F:\Images\2.bmp"... separately in new slides after the base slide.
The experimental implementation
The main entry method is InsertFigures.
Sub InsertFigures()
    'Reference: (Show object name) https://stackoverflow.com/a/52088805
    'Reference: (Replace image) https://stackoverflow.com/a/18083223

    Dim BaseSlideNumber As Integer
    Dim StartNum, EndNum As Integer
    StartNum = 1
    EndNum = 30
    BaseSlideNumber = 2
    Dim LoopNumber As Integer
    For LoopNumber = StartNum To EndNum
      Set newSlide = ActivePresentation.Slides(BaseSlideNumber).Duplicate
    Next LoopNumber

    For LoopNumber = StartNum To EndNum
        NewPictureFilename = "F:\Images\" & CStr(LoopNumber) & ".bmp"  ' Source Image Path
        Dim TargetSlideNumber As Integer
        TargetSlideNumber = BaseSlideNumber + (LoopNumber - StartNum + 1)
        Set ObjectForGettingProperties = getShapeByName("Image1", TargetSlideNumber)
        'Capture properties of exisitng picture such as location and size
        With ObjectForGettingProperties
            TopProperty = .Top
            LeftProperty = .Left
            HeightProperty = .Height
            WidthProperty = .Width
            'SoftEdgeProperty = .SoftEdge
        End With
        ObjectForGettingProperties.Delete                       ' Delete origin placeholder

        Set NewImageObject = ActivePresentation.Slides(TargetSlideNumber).Shapes.AddPicture(NewPictureFilename, msoFalse, msoTrue, LeftProperty, TopProperty, WidthProperty, HeightProperty)
        NewImageObject.Name = "NewImage"                  ' Set image name
        NewImageObject.SoftEdge.Radius = 8.86
    Next LoopNumber

End Sub

Function getShapeByName(shapeName As String, Slide As Integer)
    'Reference: https://stackoverflow.com/a/5527604
    Set getShapeByName = ActivePresentation.Slides(Slide).Shapes(shapeName)
End Function

All suggestions are welcome.

Overall, there's not much to comment on for the code logic -- it does what you need it to do. But there are some details to pay attention to that will help in this and future code.

Always use Option Explicit. There are a couple of undeclared variables/objects in your code that leads me to believe this directive is not present. Turn it on and leave it on.
Be careful when declaring multiple variables on a single line.

In your code where you have
Dim StartNum, EndNum As Integer

Then StartNum will be a Variant and EndNum will be an Integer, which is not exactly what you want. It might be a pain, but save yourself and declare each variable on a separate line (once it's a habit, it's not as bad as you think).

Declare your number variables as Long, not Integer. This answer gives a very detailed explanation, but just know it's considered a "best practice" to always use Long.
This point is more of a stylistic convention, but it's considered "standard" (personal preferences apply) to declare variables with camelCase and procedure names with PascalCase and constants with SHOUTY_SNAKE_CASE (reference). If you read that reference, PLEASE don't fall down the Hungarian case rabbit hole. (These conventions can lead to highly entertaining comments and emotional battles about the merits of these conventions. Do what I do:  get some popcorn and enjoy the show ;) )
Pay attention to the arguments in your functions or subs and identify each parameters as ByRef or ByVal. Microsoft explains the difference (using Visual Basic, but it's essentially the same in VBA), and there are other good references to read to learn more about it.
This is really my own personal preference, but I do try to limit the line length of each statement to around 80 characters. It it's longer, I add the continuation character. This is more of a guideline than a hard rule (for me), but it does make code easier to read.

The example below is my implementation of your code, as modified by my comments.
Option Explicit

Sub InsertFigures()
    'Reference: (Show object name) https://stackoverflow.com/a/52088805
    'Reference: (Replace image) https://stackoverflow.com/a/18083223

    Dim baseSlideNumber As Long
    Dim startNum As Long
    Dim endNum As Long
    startNum = 1
    endNum = 30
    baseSlideNumber = 2

    Dim loopNumber As Long
    Dim newSlide As SlideRange
    For loopNumber = startNum To endNum
      Set newSlide = ActivePresentation.Slides(baseSlideNumber).Duplicate
    Next loopNumber

    For loopNumber = startNum To endNum
        Dim newPictureFilename As String
        Dim targetSlideNumber As Long
        newPictureFilename = "F:\Images\" & CStr(loopNumber) & ".bmp"  ' Source Image Path
        targetSlideNumber = baseSlideNumber + (loopNumber - startNum + 1)

        Dim topProperty As Double
        Dim leftProperty As Double
        Dim heightProperty As Double
        Dim widthProperty As Double
        Dim softEdgeProperty As Double

        'Capture properties of exisitng picture such as location and size
        Dim objectForGettingProperties As Shape
        Set objectForGettingProperties = GetShapeByName("Image1", targetSlideNumber)
        With objectForGettingProperties
            topProperty = .Top
            leftProperty = .Left
            heightProperty = .Height
            widthProperty = .Width
            'SoftEdgeProperty = .SoftEdge
        End With
        objectForGettingProperties.Delete           ' Delete origin placeholder

        Dim newImageObject As Object
        Set newImageObject = ActivePresentation.Slides(targetSlideNumber) _
                                     .Shapes.AddPicture(newPictureFilename, _
                                                        msoFalse, msoTrue, _
                                                        leftProperty, _
                                                        topProperty, _
                                                        widthProperty, _
                                                        heightProperty)
        newImageObject.Name = "NewImage"        'Set image name
        newImageObject.SoftEdge.Radius = 8.86
    Next loopNumber
End Sub

Function GetShapeByName(ByVal shapeName As String, ByVal Slide As Long) As Shape
    'Reference: https://stackoverflow.com/a/5527604
    Set GetShapeByName = ActivePresentation.Slides(Slide).Shapes(shapeName)
End Function


Natural sorting/comparing algorithm in TypeScript This code sorts string that may contain numbers in natural order, that's it, "item 2" comes before than "item 10". It currently ignores case. I plan to implement options to handle case sensitivity and descending order.

For me it's fast enough, faster than other implementations I've seen using regexes. Apart from the first lines, it's JavaScript.

I made it returning the actual sorting function so we can cache its config object.

I'd like to know if it have any issues regarding Unicode, code style, etc. I plan to handle descending order like this, codified into a helper function.

return 0 - result;

For case sensitivity, I'd strip toUpperCase().

"use strict";

interface NaturalKeyFunc {
    (obj: any): any;
}

interface NaturalSortConfig {
    keyFunc?: NaturalKeyFunc;
    //desc?: boolean;
    //caseInsensitive?: boolean;
}

function naturalSort(config?: NaturalSortConfig) {

    return function(a, b) {

        if (typeof a === "number" && typeof b === "number")
            return a - b;

        if (a === null || b === null)
            return NaN;
        if (a === undefined || b === undefined)
            return NaN;
        if (Array.isArray(a) || Array.isArray(b))
            return NaN;

        if (Object.prototype.toString.call(a) === "[object Object]" || typeof Object.prototype.toString.call(b) === "[object Object]") {
            if (!config && !config.keyFunc)
                return NaN;
            if (typeof a === "object")
                a = config.keyFunc(a);
            if (typeof b === "object")
                b = config.keyFunc(b);
        }

        if (typeof a !== "string" || typeof b !== "string")
            return a - b;

        if (a.length === 0 || b.length === 0)
            return a.length - b.length;

        var s1: string = a,
            s2: string = b,
            indexA = 0, indexB = 0,
            numberA, numberB,
            charA, charB,
            difference;

        do {
            numberA = 0;
            while (indexA < s1.length) {
                charA = s1[indexA];
                indexA++;
                if (!(charA >= "0" && charA <= "9")) break;

                numberA = numberA * 10 + parseInt(charA);
            }

            numberB = 0;
            while (indexB < s2.length) {
                charB = s2[indexB];
                indexB++;
                if (!(charB >= "0" && charB <= "9")) break;

                numberB = numberB * 10 + parseInt(charB);
            }

            if (charA === " " && charB === " ") {
                while (indexA < s1.length && s1[indexA] === " ") {
                    indexA++;
                }
                while (indexB < s2.length && s2[indexB] === " ") {
                    indexB++;
                }
            }

            difference = numberA - numberB;
            if (difference !== 0) return difference;
            if (charA !== charB) {
                if (charA === ".") return -1;
                if (charB === ".") return 1;
                return charA.toUpperCase().charCodeAt(0) - charB.toUpperCase().charCodeAt(0);
            }
        } while (indexA < s1.length && indexB < s2.length);

        return (indexB - s2.length) - (indexA - s1.length);
    }
}

Some tests:

let natu = naturalSort();

it("can sort numbers inside strings", function () {
    expect(natu("a1", "a2")).toBeLessThan(0);
    expect(natu("1 item", "1 item")).toEqual(0);
    expect(natu("item 1", "item 1")).toEqual(0);
    expect(natu("item 1", "item 2")).toBeLessThan(0);
    expect(natu("item 1", "item 10")).toBeLessThan(0);
    expect(natu("item 2", "item 10")).toBeLessThan(0);
    expect(natu("item 1 item a", "item 1 item a")).toEqual(0);
    expect(natu("item 1 item 1", "item 1 item 10")).toBeLessThan(0);
    expect(natu("item 01", "item 1")).toEqual(0);
});

Dead code

interface NaturalSortConfig {
    keyFunc?: NaturalKeyFunc;
    //desc?: boolean;
    //caseInsensitive?: boolean;
}

Dead code does nothing, except take up screen space, and confuse people who don't know why it was written and removed in the first place.

Braces

This is a bit of a holy-war.  However, at least one bug (the Apple SSL bug) was caused by missing braces.  Personally, I use them:

if (typeof a === "number" && typeof b === "number") {
    return a - b;
}

Instead of:

if (typeof a === "number" && typeof b === "number")
    return a - b;

Functions

You may want to consider splitting this into more methods.  One possible method might be to move the code to determine whether to early-return into its own function:

function IsValidData (o) {
    return o === null || o === undefined || Array.isArray(o);
}

Most of the code in naturalSort() is duplicated - more functions would really help here.

Early exit

As mentioned in the comments in the previous (deleted) answer, you can early-return as soon as you know which value is sorted higher.  For example, sorting "aa" against "bb" should be able to return after checking the first character of each, while "aaa" vs. "aba" should be able return after checking the second, and so on.


Automatic picture selector I'm working on a project for digital marketing, I want to automate the upload of pictures. The program itself isn't done but here I came up with a way to keep track of which and when pictures have been posted.

from os import listdir, rename
from random import choice
from _datetime import datetime

# The main directory in which I'll store the pictures
pictures_dir = "C:/Users/firmi/Desktop/Pictures/"

# The directory in which posted pictures go to
pictures_posted_dir = pictures_dir + "Posted/"

# The folders inside the main directory
picture_folders = listdir(pictures_dir)

# List of folders with pictures that have not been posted
pictures_to_post = [kind for kind in picture_folders if not kind.startswith('Posted') and len(listdir(pictures_dir + kind)) > 0]

# Current date of the post
post_time = datetime.now().strftime("_(%d-%m-%Y)")

def post_picture(picture_dir, kind):
    # Here I move the picture to the 'posted' folder
    rename(picture_dir, pictures_posted_dir + kind + str(post_time) + ".png")

def randomize_picture():
    # Here a random picture is selected(if there are any)
    if len(pictures_to_post) == 0:
        print('No Pictures Left')
    else:
        random_kind = choice(pictures_to_post)
        random_kind_dir = pictures_dir + random_kind + "/"
        random_picture = choice(listdir(random_kind_dir))
        random_picture_dir = random_kind_dir + random_picture

        post_picture(random_picture_dir, random_kind)

randomize_picture()

It's all very generic because I want to be able to have multiple 'main folders' and they are all going to have their kinds of pictures.
Is there any way I can improve this code?

I'd recommend having a look at Python's "official" Style Guide aka PEP 8, which is widely accepted in the community as a guideline on how Python code should be formatted.

But you can also learn a lot from looking at other peoples code, e.g. here on Code Review. E.g. it's very common to have imports at the top of the file, followed by constants, class and function definitions, with "script" code most often found at the very end. The script code is often surrounded by if __name__ == "__main__":, which is a way to tell the interpreter to only run this piece of code in case the file is executed as a script (and not when you try to import a function from it).

Directly applying this to your code might leave you with an awkward feeling, because some global variables are used inside the functions before they are "physically" defined in the script. You've probably heard this from other people already: try to avoid global variables whenever possible. This is one of the reasons. In general, global variables make it harder so see what goes into a function and how the function affects the "state"/variables of your program without looking at the source code. If all inputs are passed as parameters and the new/altered values are actually returned from the function, you don't need to know what exactly happens inside to understand what's going on.

You work around this be passing all the necessary variables as parameters when calling a function.

If that brings up another strange feeling, like say, a function accepts parameters that seem to have nothing to do with what it name would let one expect, then you have identified another common error: violations of the single responsibility principle. A few ideas how that might look like in your script

Have a main() function where all the script code lives. This function calls other functions as necessary and provides input to them via parameters.
Have randomize_picture accept all the necessary inputs as parameter and return which image was selected, raise an appropriate exception whenever this is not possible instead of printing to console.
Pass the source image and all the other relevant information to post_image(...).
Alternatively, assemble the target filename also in main so you'll just have to pass two parameters to post_image(...).

import os
from os import listdir, rename
from random import choice
from datetime import datetime

def post_picture(original_picture, pictures_posted_dir, kind, post_time):
    """Here I move the picture to the 'posted' folder"""
    rename(original_picture, pictures_posted_dir + kind + post_time + ".png")

def randomize_picture(pictures_to_post, pictures_dir):
    """Here a random picture is selected(if there are any)"""
    if not pictures_to_post:
        raise ValueError('No Pictures Left')

    random_kind = choice(pictures_to_post)
    random_kind_dir = pictures_dir + random_kind + "/"
    random_picture = choice(listdir(random_kind_dir))
    random_picture_dir = random_kind_dir + random_picture

    return random_picture_dir, random_kind

def main():
    # The main directory in which I'll store the pictures
    pictures_dir = "C:/Users/firmi/Desktop/Pictures/"

    # The directory in which posted pictures go to
    pictures_posted_dir = os.path.join(pictures_dir, "Posted/")

    # The folders inside the main directory
    picture_folders = listdir(pictures_dir)

    # List of folders with pictures that have not been posted
    pictures_to_post = [kind for kind in picture_folders if not kind.startswith('Posted') and len(listdir(pictures_dir + kind)) > 0]

    # Current date of the post
    post_time = datetime.now().strftime("_(%d-%m-%Y)")

    random_picture_dir, random_kind = randomize_picture(pictures_to_post, pictures_dir)

    post_picture(random_picture_dir, pictures_posted_dir, random_kind, post_time)

A few notes on what else has changed:

import _datetime --> impor datetime. Variables, functions and modules starting with a leading _ are not supposed to be used in code outside of that file or module. It's a Python convention to tell others, that you consider it an implementation detail which is subject to change. So don't use it.
the function documentation was changed from # comments to proper docstrings. docstrings are found by Python's built-in help(...) function as well as by most IDEs and are THE way to go for function, class, and module documentation.
whenever working with paths it's best to use either os.path.join (Python 2/3) or the pathlib module. Both of them make working with paths much more convenient, e.g. because they know which separator to use for which platform (Linux: /, Windows: \).
Look Ma! No global variables ;-)


Optimise nested loops used to compare champions which have abilities which have effects I'm creating a counter picker for a game called League of Legends, and for this I need to compare effects of abilities, which each champion has. For this I'm using 4 lists of champions, the returnList, the banned champion list, the enemy champions list, annd the allied champion lists.

However, seeing that the returnlist, where the champions get there points, contains around 120 instances, which all have 5 abilities, which in turn have around 5 effects each, I feel that my current solution of nesting foreach loops might be really slow.

I was wondering how I could best optimise this

foreach (CounterChampion champion1 in enemyList)
{
    foreach (CounterAbility ability1 in champion1.Abilities)
    {
        foreach (string effects1 in ability1.Effects)
        {
            foreach (CounterChampion champion2 in returnList)
            {
                foreach (string effects2 in champion2.Abilities.SelectMany(ability2 => ability2.Effects))
                {
                    if (CounterEffectExists(effects2, effects1, CounterEffect.EffectType.Counters))
                    {
                        champion2.AddPoints(1);
                    }
                    if (CounterEffectExists(effects1, effects2, CounterEffect.EffectType.Counters))
                    {
                        champion2.AddPoints(-1);
                    }
                }
            }
        }
    }
}
foreach (CounterChampion champion1 in banList)
{
    foreach (CounterAbility ability1 in champion1.Abilities)
    {
        foreach (string effects1 in ability1.Effects)
        {
            foreach (CounterChampion champion2 in returnList)
            {
                foreach (string effects2 in champion2.Abilities.SelectMany(ability2 => ability2.Effects))
                {
                    if (CounterEffectExists(effects1, effects2, CounterEffect.EffectType.Counters))
                    {
                        champion2.AddPoints(1);
                    }
                }
            }
        }
    }
}
foreach (CounterChampion champion1 in allyList)
{
    foreach (CounterAbility ability1 in champion1.Abilities)
    {
        foreach (string effects1 in ability1.Effects)
        {
            foreach (CounterChampion champion2 in returnList)
            {
                foreach (string effects2 in champion2.Abilities.SelectMany(ability2 => ability2.Effects))
                {
                    if (CounterEffectExists(effects2, effects1, CounterEffect.EffectType.WorksWith))
                    {
                        champion2.AddPoints(1);
                    }
                    if (CounterEffectExists(effects1, effects2, CounterEffect.EffectType.WorksWith))
                    {
                        champion2.AddPoints(1);
                    }
                }
            }
        }
    }
}
returnList.Sort(
    delegate (CounterChampion c1,
    CounterChampion c2)
    {
        return c1.GetPoints().CompareTo(c2.GetPoints()) * -1;
    }
);
return returnList;

Well the inner loop is something like 120*5*5 => 1200 cycles and I really don't know if it can become an issue with today's PC power. But improvements can always be made, and reading your code, I see that some lines are duplicated :

   foreach (CounterChampion champion2 in returnList)
    {
        foreach (string effects2 in champion2.Abilities.SelectMany(ability2 => ability2.Effects))
        {

As as first step toward performance and code structuration, I suggest to compute the ability/effect in returnList once, and reuse it after. Typically, you may have something like this :

class EffectByChampion{
  CounterChampion Champion,
  List<string> Effects
}

then you may fill the following list ONCE

List<EffectByChampion>

and after you may reuse it 3 times through your existing code with :

    foreach (string effects1 in ability1.Effects)
    {
      foreach(EffectByChampion effectByChampion in EffectByChampionList)
      {
       foreach(string effects2 in effectByChampion)

This way, you have 3 foreach loops removed.
Hope this help !