Replacing variables with actual data I've written a class for loading a template file and replacing the variables with actual data. Please review my code and suggest improvements.

<?php
/**
 * This class will helpful for variable replacement from a loaded file
 *
 * @author V V R
 * @version 1.0
 */
class Parser {
    public $dir = "";
    public $text = "";

    /**
     * To get directory location
     * @param $dir
     */
    function __construct($dir = "home/template") {
        $this->dir = $dir;
    }

    /**
     * To load the specified file from directory
     * @param $file
     */
    function loadFile($file) {
        try {
            if (file_exists ( $this->dir . "/" . $file )) {
                $this->text = file_get_contents ( $this->dir . "/" . $file );
            } else {
                throw new Exception ( $this->dir . "/" . $file . " does not exist" );
            }
        } catch ( Exception $e ) {
            error_log ( $e->getMessage () );
            die ();
        }
    }
    /**
     * To get the text from a file
     * @return text
     */
    function getText() {
        return $this->text;
    }

    /**
     * To replace the variables
     * @param unknown_type $var
     * @param unknown_type $text
     */
    function replace($var, $text) {
        $this->text = str_replace ( "{\$$var}", $text, $this->text );
    }
}

?>

Note: I'm sorry if this is completely irrelevant but I'm not entirely sure what you are trying to accomplish. If I'm way off base please let me know.

When I create a template loader I generally use output buffering with php include. This allows you to run the page as a php file without displaying its content before you are ready. The advantage to "parsing" your php files this way is that you can still run loops and functions.

This is how your loadFile method would look.

<?php

function loadFile($file, array $variables = null) {
  try {
      if (file_exists ( $this->dir . "/" . $file )) {

          // start output buffering
          ob_start();

          // create variables from the key/value pairs in the array
          if( is_array($variables) ) {
              extract($variables);
          }

          include_once $this->dir . "/" . $file;

          // store the output
          $this->text = ob_get_clean();

          // $this->text = file_get_contents ( $this->dir . "/" . $file );
      } else {
          throw new Exception ( $this->dir . "/" . $file . " does not exist" );
      }
  } catch ( Exception $e ) {
      error_log ( $e->getMessage () );
      die ();
  }
}

?>

As I said before, I'm not sure what kind of templates you are trying to load, but hopefully this will help.


Merge elements in an array I have written two different codes for merging (merging with costs) and I want to know if I can make optimizations to run faster.

merge1:

 private static long merge(List<Integer> array, int[] res, int start, int mid, int end) {

        int i = start; // index of left subarray
        int j = mid; // index of right subarray
        int k = start; //index of mid

        long inversions = 0;

        for (int r = start; r <= end; r++) {

            if ((j > end) || (i < mid && array.get(i) <= array.get(j))) {
                res[r] = array.get(i);
                i++;
            } else {

                if (i < mid) {

                    if (array.get(i) - array.get(j) > 1) {

                        inversions += 3 * (mid - i);
                    } else {
                        int s;
                        s = MostRightSmallestElement.search(array, i, mid - 1);
                        inversions += 2 * (s - i + 1);
                        inversions += 3 * (mid - (s + 1));
                    }

                }
                res[r] = array.get(j);
                j++;

            }
        }

       //Copy the remaining elements of left subarray  (if there are any) to res
        while (i <= mid - 1) {
            res[k++] = array.get(i++);
        }

        //Copy the remaining elements of right subarray  (if there are any) to res
        while (j <= end) {
            res[k++] = array.get(j++);
        }

        //Copy back the merged elements to original array
        for (i = start; i <= end; i++) {
            array.set(i, res[i]);
        }

        return inversions;

    }

merge2:

private static long merge(List<Integer> array, int[] res, int start, int mid, int end) {

        int i = start; // index of left subarray
        int j = mid; // index of right subarray
        int k = start; //index of mid

        long inversions = 0;

        while( ( i <mid) && (j <= end )) {

            if( array.get(i) <=  array.get(j)){
                res[k++] = array.get(i++);
            }
            else {

                if (i != mid) {

                    if (array.get(i) - array.get(j) > 1) {

                        inversions =  inversions + 3 * (mid - i);

                    } else {
                          int s;
                        s = MostRightSmallestElement.search(array, i, mid - 1);
                        inversions =  inversions +  2 * (s - i + 1);
                        inversions =  inversions +  3 * (mid - (s + 1));
                    }
          }
                res[k++] = array.get(j++);

        }
        }

       //Copy the remaining elements of left subarray  (if there are any) to res
        while (i <= mid - 1) {
            res[k++] = array.get(i++);
        }

        //Copy the remaining elements of right subarray  (if there are any) to res
        while (j <= end) {
            res[k++] = array.get(j++);
        }

        //Copy back the merged elements to original array
        for (i = start; i <= end; i++) {
            array.set(i, res[i]);
        }

        return inversions;

    }

Any ideas how I can optimize the code? I count the inversions but the general
idea of merging is the same.

comparative-review:
In both variants, both "traversing indices" get compared to their (respective upper) limit in each traversal of the "proper merge loop".
While "the copy2res-loops" should never execute in merge1, the "res-index" gets compared to its limit right from the start.
In merge2, the "proper merge loop" isn't traversed as often.
Exactly one of the copy2res-loops should be traversed for the same total.

remarks:

merge() isn't doc-commented.
the comments for start&mid might as well be @params,
index of mid looks off,
all three may be final.
Both subarrays include their start - excluding end would be consistent.
I don't get the inversion counting - lack of documentation doesn't help.

Speed-up:

beware memory hierarchy effects
old school complexity analysis assumes uniform memory access time - far from what I'd want to pay and wait for.
don't copy back and fro. Use the auxiliary sequence as the/one source for the next pass.
don't convert between Integer and int time and again.
when using Lists, avoid relying on the "random accessors" to be fast
(implementing RandomAccess is a hint - read backwards, its doc recommends to "always")
use ListIterators to iterate Lists  (note how they (optionally) support set(newValue))
don't time-consumingly ponder/implement variants unless you

established it matters
fixed a measure for good enough

converting to arrays and never looking back (with the possible exception of a terminating conversion/copy back) is an option promising sequential storage and allowing bulk operations. With Lists, operations on sublists may be more efficient.
Most JREs allow a peek at their implementation (have a look at java.util.Arrays, too).


Encrypting an AES key This encryption class RSA encrypts an AES key and then when the encrypt method is called it decrypts the AES key and uses the key to encrypt the message and basically the same thing with the decryption.

However I am very very new to cryptography so I do not know if this is considered "good" or if this could be improved drastically.

--

What I would like to be reviewed:

Is this considered secure method to cipher messages
Is this efficient
Is this code "clean"

import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.SecureRandom;

import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;

public class Cryptographer {
    // CIPHER NECESITIES:
    private KeyPairGenerator KEY_PAIR_GENERATOR;
    private KeyGenerator KEY_GENERATOR;
    private KeyPair RSA_KEYS;
    private byte[] AES_KEY_DATA;
    private Cipher AESKeyEncryptor;
    private Cipher MessageEncryptor;
    private IvParameterSpec Iv = new IvParameterSpec(SecureRandom.getSeed(16));

    // SETTINGS:
    final private int RSA_KEY_SIZE = 2048;
    final private int AES_KEY_SIZE = 256;
    final private String RSA_ALGORITHM = "RSA/ECB/OAEPWithSHA-256AndMGF1Padding";
    final private String AES_ALGORITHM = "AES/CBC/PKCS5PADDING";

    // CONSTRUCTOR:
    public Cryptographer() throws Exception {
        KEY_PAIR_GENERATOR = KeyPairGenerator.getInstance("RSA");
        KEY_GENERATOR = KeyGenerator.getInstance("AES");
        KEY_PAIR_GENERATOR.initialize(RSA_KEY_SIZE);
        KEY_GENERATOR.init(AES_KEY_SIZE);
        RSA_KEYS = KEY_PAIR_GENERATOR.generateKeyPair();

        MessageEncryptor = Cipher.getInstance(AES_ALGORITHM);
        AESKeyEncryptor = Cipher.getInstance(RSA_ALGORITHM);
        AES_KEY_DATA = EncryptAESKey(KEY_GENERATOR.generateKey());
    }

    // PRIVATE METHODS:
    private byte[] EncryptAESKey(SecretKey key) throws Exception {
        AESKeyEncryptor.init(Cipher.ENCRYPT_MODE, RSA_KEYS.getPublic());
        return AESKeyEncryptor.doFinal(
                key.getEncoded()
            );
    }

    private SecretKey DecryptAESKey(byte[] EncryptedAESKey) throws Exception {
        AESKeyEncryptor.init(Cipher.DECRYPT_MODE, RSA_KEYS.getPrivate());
        return new SecretKeySpec(
                AESKeyEncryptor.doFinal(EncryptedAESKey),
                "AES"
            );
    }

    // PUBLIC METHODS:
    public byte[] Encrypt(byte[] message) throws Exception {
        MessageEncryptor.init(Cipher.ENCRYPT_MODE, DecryptAESKey(AES_KEY_DATA), Iv);

        return MessageEncryptor.doFinal(message);
    }

    public byte[] Decrypt(byte[] message) throws Exception {
        MessageEncryptor.init(Cipher.DECRYPT_MODE, DecryptAESKey(AES_KEY_DATA), Iv);

        return MessageEncryptor.doFinal(message);
    }
}
```

You're trying to create a so called hybrid cryptosystem or hybrid encryption method (crypto is not just considered encryption anymore, after all).

The problem with CBC mode is that it is very vulnerable to padding oracle attacks. It is itself not authenticated, which means that you can change any data within the ciphertext and thereby change plaintext blocks. Using GCM mode is recommended.

Adding a digital signature over the message is recommended if you want to know who send the message to you. Currently your code is only valid for providing confidentiality for locally stored data (data "at rest" in the jargon).

The whole idea of hybrid encryption is to send the encrypted AES key as part of the ciphertext. Since the size of the encrypted AES key is always the same size as the key size of the RSA key pair in bytes, it can simply be prefixed to the AES ciphertext.

Of course, you'd need to split the resulting ciphertext message in two again to be able to decrypt it. Note that the Java API misses a method to retrieve the RSA key size. This is however OK because you can retrieve the modulus size by using ((RSAPrivateKey)RSA_KEYS.getPrivate()).getModulus().bitLength() in your code. This is - by definition - the same as the key size in bits.

Your code is clearly not keeping to any conventions when it comes to capitalization. This is however already mentioned in the other answer, so I don't see any reason to repeat it here.

So lets make a list of what's wrong:

first of all, the RSA key pair is the static key pair. That means that it should be pre-generated, and the public key must be distributed and trusted in advance. If the RSA and data specific AES key are generated in the same method (in this case the constructor), then something is seriously wrong.
Cipher instances carry state, and should preferably not be stored in fields. Storing static keys such as the RSA key pair is OK, but otherwise everything should just be created within the various methods. Cipher instances are relatively lightweight, so constructing / deconstructing them is relatively efficient (certainly compared against their operation). All in all, only the key pair field makes some sense; all the rest is unnecessary state.
The RSA key size of 2048 has about the same security as a 112 bit cipher such as 3DES. That's lower than AES-128 and much lower than AES-256. I'd use at least a 3072 bit key and prefer a 4096 bit key. Above that Elliptic Curve cryptography (ECIES) starts to make more sense (or a post-quantum algorithm).
SecureRandom.getSeed(16) retrieves data that can be used to seed a random number generator. Using a truly random IV is (more or less) required for CBC mode, but it is actually more secure and more performant to use SecureRandom#nextBytes.

Other smaller notes:

the name of the class: "Cryptographer" is of course horrible and it doesn't explain what the class does. This class does not represent a person.
It's "necessary" not "necesary". The shouting makes such spelling mistakes all the worse.

All in all, nice as a first try, don't use in production.


Find the largest decrease in a sequence of numbers I have written a function that takes as input a list of 5 numbers, then the program's goal is to return the largest drop in the list. It is important to understand that [5, 3, ...] is a decrease of 2 and that [5, 20, .....] is a decrease of 0. In short, a decrease can only be from left to right and the largest decrease can for example also be the decrease from the 1st number to the 4th number, they do not necessarily have to be next to each other.
def biggest_drop(temperature):
    difference = 0
    new_temperature = temperature.copy()
    a = 0
    for i in temperature:
        del new_temperature[0]
        for j in new_temperature:
            if i <= j:
                   False
            else:
                a = i - j
                if a > difference:
                    difference = a
    return difference

The good news is the code works, the bad news is that because of the double for loop there is already a Big-O notation of O(n2). However, the program must have a Big-O notation of O(n1) and I don't know how to fix this and where the speed of the code is also high enough. For example, here I have made a fairly sloppy alternative, but it is too slow:
def biggest_drop(temperature):
    difference = 0
    a = 0
    b = temperature[1]
    c = temperature[2]
    d = temperature[3]
    e = temperature[4]
    for i in temperature:
        if temperature[0] <= temperature[1] <= temperature[2] <= temperature[3] <= temperature[4]:
            return 0
        elif i > b:
            a = i - b
            if a > difference:
                difference = a
        elif i > c:
            a = i - c
            if a > difference:
                difference = a
        elif i > d:
            a = i - d
            if a > difference:
                difference = a
        elif i > e:
            a = i - e
            if a > difference:
                difference = a
    return difference

Do you have a better idea how my code complies with the big-O notation of O(n1) and is also fast enough for larger n, in other words lists with more than 5 integers?

This can be solved in a single pass, e.g., O(n).
For each temperature, subtract it from the highest temperature seen so far to get the drop.  Keep track of the largest drop.  Also update the highest temperature.
def biggest_drop(temperatures):
    highest = temperatures[0]
    largest_drop = 0

    for temperature in temperatures:
        drop = highest - temperature
        largest_drop = max(largest_drop, drop)

        highest = max(temperature, highest)

    return largest_drop


Return true if the elements of an array do not contain one or the other I am gradually completing the CodingBat exercises for Java. Here is the one I just did:

  Given an array of ints, return true if it contains no 1's or it contains no 4's.

Here is my code:

public boolean no14(int[] nums) {

    int oneCount = 0;
    int fourCount = 0;

    for (int i = 0; i < nums.length; i++) {
        if (nums[i] == 1) {
            oneCount++;
        }
        if (nums[i] == 4) {
            fourCount++;
        }
    }

    if (oneCount > 0 && fourCount > 0) {
        return false;
    }
    return true;
}

Now, I am pretty unfamiliar with arrays, so I would just like to know if there is a simpler/shorter way of finding such information without setting a count for each number? Perhaps using a regular expression?

In JDK there's nothing, but with Guava you could do

List<Integer> list com.google.common.primitives.Ints.asList(nums);
return !(list.contains(1) && list.contains(4));

Obviously, converting int[] to something like ArrayList<Integer> would be a performance disaster. Fortunately, there's nothing like this, instead there's a class implementing the List interface by accessing the primitive array. Bulk-methods like contains are optimized and lead to a simple loop with no boxing inside.

But seriously, feel free to stick with your loop, as there's not always such a trick available. What you could improve:

You need no xxxCount, a simple boolean containsXxx would do. But this is no problem, having the count may be useful sometimes.
You could use an early return. Whenever you find a 1, you can check if you have already a 4 and return false if so.

The only part I dislike is

if (oneCount > 0 && fourCount > 0) {
    return false;
}
return true;

You surely mean

return oneCount == 0 || fourCount == 0;

If performance was very important (which it rarely is), I'd try this

int i = 0;
int missing;
for (; ; i++) {
    if (i == nums.length) {
        return true;
    } else if (num[i] == 1) {
        missing = 4;
        break;
    } else if (num[i] == 4) {
        missing = 1;
        break;
    }
}
for (; i < nums.length; i++) {
    if (nums[i] == missing) {
        return false;
    }
}
return true;

But as you can see, it's terribly long and not very readable. When embedded in a real code it may also be slower (due to inlining limit or whatever).


Non-recursive factorial in Ruby I've implemented a factorial function in a more "Ruby" way. I would like to get feedback on the algorithm.

def factorial_of n
  (1..n).inject :*
end

factorial_of 5
=> 120

The "of" in the name factorial_of is redundant and unconventional.

factorial_of 0 returns nil instead of the correct answer, which is 1.

def factorial(n)
  (1..n).inject(1, :*)
end


Function to convert little endian bytes into big endian I'm working on a little endian machine and in my program I need to convert 2,3,4,5 or 8 bytes into network order before transmitting them over the network. I've written the following function which basically just reverses the bytes. I just wanted to make sure if it's logic is correct.

 void convertLittleToBig(const uint8_t* in, uint8_t* out, const uint64_t& sizeInBytes) {
      for(int i=0;i<sizeInBytes;++i)
        out[i] = in[sizeInBytes-i-1];
    }

The function assumes that 'out' and 'in' will be pointing at 'sizeInBytes' memory.
If I'm not wrong, I can use the same function to convert from network order into little endian. Basically big endian and little endian, are mirror images of each other in terms of their byte order as per my current understanding.

Given below is a sample program:

#include <iostream>
#include <cstdint>
#include <iostream>

using std::cin;

//This function prints the raw bytes in memory.

void printBytes(const uint8_t* p, const uint64_t size) {
for(int i=0;i<size;++i)
  printf("\nbyte[%d](%x)",i+1,p[i]);
printf("\n");
}

void convertLittleToBig(const uint8_t* in, uint8_t* out, const uint64_t& sizeInBytes) {
  printf("\nCurrent representation\n");
  printBytes(in,sizeInBytes);
  for(int i=0;i<sizeInBytes;++i)
    out[i] = in[sizeInBytes-i-1];
  printf("\nRepresentation after conversion\n");
  printBytes(out,sizeInBytes);
}

int main() {
  uint64_t little;
  printf("\nEnter a number that you wish to convert into big endian: ");
  int64_t inp;
  cin>>inp;
  little=inp;
  uint64_t big;
  convertLittleToBig((uint8_t*)&little,(uint8_t*)&big,sizeof(big));
}

Includes
We have included <iostream> twice, but missed <cstdio>.
Misspelt standard library identifiers
std::printf, std::uint8_t and std::uint64_t are consistently misspelt.  You might sometimes get away with this, as your standard library is allowed to add global-namespace versions of those identifiers; since it's not required to do so, you have a portability bug.
Use appropriate size type
std::size_t is the appropriate type to use for the size of an object, rather than std::uint64_t (which could be unnecessarily big, or - theoretically, at least for now - too small).
Conversion function shouldn't have side-effects
The printf() calls within convertLittleToBig make it unusable in any serious program.
Consider a standard algorithm
The <algorithm> header provides a very useful std::copy() function we can use if we have a reverse iterator to copy from or to.  We can get suitable iterators from std::span views onto the inputs (from C++20 onwards).
Avoid raw pointers
We could do better, using a template to accept values and return by value, inferring the size from the argument type:
#include <algorithm>
#include <span>

template<typename T>
T convertLittleToBig(const T& val)
{
    auto in = std::as_bytes(std::span(&val, 1));
    T result;
    auto out = std::as_writable_bytes(std::span(&result, 1));
    std::copy(in.rbegin(), in.rend(), out.begin());
    return result;
}

That's much simpler - no counting needed, and the caller doesn't need to use sizeof (to work with odd-sized values, they need to be passed as arrays).
This is how we'd use it in a simple test program:
#include <cstddef>
#include <cstdlib>
#include <iomanip>
#include <iostream>
#include <sstream>
#include <string>

std::string to_hex(auto const& val)
{
    std::ostringstream oss;
    oss << std::hex << std::setfill('0');
    auto in = std::as_bytes(std::span(&val, 1));
    for (auto c: in) {
        oss << std::setw(2) << std::to_integer<unsigned int>(c);
    }
    return oss.str();
}

// hex-print the value and its reversal
void demo_endian_swap(auto const& val)
{
    std::cout << to_hex(val) << " -> "
              << to_hex(convertLittleToBig(val)) << '\n';
}

#include <array>
int main() {
    // Demonstrate a selection of types
    demo_endian_swap(std::uint16_t{0x1234});
    demo_endian_swap(std::uint64_t{0x123456789abcde});
    demo_endian_swap(std::array<unsigned char,5>{1,2,3,4,5});
    //demo_endian_swap("abc");    // Invalid - can't return an array
    //demo_endian_swap(std::string{"abc"}); // oops - reverses whole structure
}

Example output:
3412 -> 1234
debc9a7856341200 -> 00123456789abcde
0102030405 -> 0504030201


std::lock implementation in C with pthreads I messed a little bit with pthreads and needed an alternative to the C++11 function std::lock (2 args are enough), and this is what I came up with:

void lock(pthread_mutex_t* m1, pthread_mutex_t* m2) {
  while(1) {
    pthread_mutex_lock(m1);

    if(pthread_mutex_trylock(m2) == 0) { // if lock succesfull
      break;
    }
    pthread_mutex_unlock(m1);
    sched_yield();
  }
}

It works fine so far.  I am just not sure whether I missed some potential deadlock.

In addition, I am interested if I should do some more error checking.  I am especially not that firm with C-style error checking.

It does not look like your code will deadlock.
That said, I am not very keen on the spin lock like attempt, as I can see situations where it may not be great in efficiency.
It (std::lock) guarantees that no matter what order you specify the locks in the parameter list you will not fall into a deadlock situation.
A common technique (simple enough you could use (but the standard does not specify whether this technique is used by std::lock)) is to make sure the locks are ordered in some way. This basically means that the order you lock the mutexes in must be consistent (thus independent of the order they are in the parameter list).

The actual order should be defined in terms of some immutable property of the underlying mutex (such as its address).
This also means that if a lock in the list is already locked it must be released so that the locks are acquired in the correct order (Note: not all lockables can safely call try_lock() so you can't always determine if you should unlock).

Example of deadlock occurring when locks are not acquired in order:
 Time:     Thread-1          Thread-2
           lock(g2, g1)      lock(g1, g2)

   0       lock(&g2)

   1                         lock(&g1)

   2       lock(&g1) // Can never succeed

   3                         lock(&g2) // Can never succeed

Metal deadlock.
Thread 1 is now waiting for g1 (held by thread 2), while thread 2 is waiting for g2 (held by thread 1).


Linked object for creating chains without lists I need a LinkedList but unfortunatelly the one that the .NET provides didn't have the functionality I was looking for (not working with linq and the LinkedListNode is sealed). I created a replacement that works similar but is linq-compatible.

The main class is a LinkedObject<T> that might be used on it's own but is better suited to be used by composition and should provide all the functionality of a chain:

[DebuggerDisplay("Value = {Value?.ToString()}")]
class LinkedObject<T>
{
    private LinkedObject<T> _previous;
    private LinkedObject<T> _next;

    public LinkedObject() {}

    public LinkedObject(T value)
    {
        Value = value;
    }

    public LinkedObject<T> Previous
    {
        get { return _previous; }
        set
        {
            _previous = value;
            value._next = this;
        }
    }

    public LinkedObject<T> Next
    {
        get { return _next; }
        set
        {
            _next = value;
            value._previous = this;
        }
    }

    public T Value { get; set; }

    // or maybe precessors?
    public IEnumerable<LinkedObject<T>> Before
    {
        get
        {
            var item = this;
            while (item != null)
            {
                yield return item;
                item = item.Previous;
            }
        }
    }

    // or maybe successors?
    public IEnumerable<LinkedObject<T>> After
    {
        get
        {
            var item = this;
            while (item != null)
            {
                yield return item;
                item = item.Next;
            }
        }
    }

    public void Remove()
    {
        _previous.Next = _next;
        _next.Previous = _previous;
        _previous = null;
        _next = null;
    }
}

Some custom object would use it like this (my real linked-object has a few more additional properties then this simple example):

class Word
{
    private readonly LinkedObject<Sentence> _linkedObject;

    public Word()
    {
        _linkedObject = new LinkedObject<Sentence>(this);
    }

    public string Value
    {
        get { return _linkedObject.Value; }
        set { _linkedObject.Value = value; }
    }

    public WordPrevious
    {
        get { return _linkedObject.Previous.Value; }
        set { _linkedObject.Previous = value._linkedObject; }
    }

    public Word Next
    {
        get { return _linkedObject.Next.Value; }
        set { _linkedObject.Next = value._linkedObject; }
    }

    public IEnumerable<Word> Before
    {
        get { return _linkedObject.Before.Select(x => x.Value); }
    }

    public IEnumerable<Word> After
    {
        get { return _linkedObject.After.Select(x => x.Value); }
    }
}

Example (I use a loop to create the real chain):

var foo = new LinkedObject<string>
{
    Value = "foo",
    Next = new LinkedObject<string>
    {
        Value = "bar",
        Next = new LinkedObject<string>() { Value = "baz" }
    }
};

// foo, bar, baz

foo.Next.Remove(); // foo, baz

Rule your own Find extension method instead of redoing it all over again.

public static LinkedListNode<T> Find<T>(this LinkedList<T> list, Predicate<T> pred)
{
    var node = list.First;
    while (node != null)
    {
        if (pred(node.Value))
        {
            return node;
        }
        node = node.Next;
    }
    return null;
}


Finding the best terms to use I have a real-world problem involving finding the best known term from a string containing sets of terms. We have Term objects that are comparable in that they have a method a_term.is_better_than(other_term). This method is Boolean -- if it returns False, then a_term may be as good as other_term, but isn't better.

Using this method, I need to pare down a set of terms to the best ones. The best way I've been able to think about this is to compare pairs of terms and eliminate any terms when one isn't as good as the other, something like this:

def get_best_terms(terms: Set[Term]): Set[Term]
    best_terms = set()
    m_terms = list(terms)
    while m_terms:
        term = m_terms.pop()
        for i in reversed(range(len(m_terms))):
            if term.is_better_than(m_terms[i]):
                del m_terms[i]
            if m_terms[i].is_better_than(term):
                break
        else: # We never encountered the `break` statement
            best_terms.add(term)
            break
    best_terms |= {a_term for a_term in m_terms
                   if not (term.is_better_than(a_term) or
                           a_term.is_better_than(term))}
    return best_terms

This approach is functional, but pretty gross in my opinion. For one thing, there's a lot of mutation going on here. For another, once I've identified a single best term, finding all the terms that are equally good is messy. The whole thing is hard to read.

What do you suggest?

Examples

A Term is a string with some metadata attached. For example,

a = AutoTerm(term='honda', term_span=1, term_range=(1, 2),
            original_term='CAR HONDA ACC...')
b = AutoTerm(term='2015 honda accord', term_span=2, term_range=(1, 3),
             original_term='2015 HONDA ACC...')
c = AutoTerm(term='2015 accord', ...)

Sometimes a term is a better choice because it's more specific, but there are other nuances, such as a match in a database. Let's say, arbitrarily, that b and c are equally good, but a is not as good. Then

assert get_best_terms({a, b, c}) == {b, c}

should pass.

You have a bug, if you remove an item in your for loop without breaking you get an IndexError.
Instead use a worse set so that you know what to skip.
What you seem to be doing is performing a sort, and so if you make a cmp function you can use sorted.
To do this you need to use functools.cmp_to_key and then you can itertools.takewhile.

And so I'd use:

import functools
import itertools
from typing import Set, List

class Term(int):
    def is_better_than(self, other):
        return self // 2 > other // 2

def cmp(self: Term, other: Term):
    if self.is_better_than(other):
        return 1
    if other.is_better_than(self):
        return -1
    return 0

def get_best_terms(terms: Set[Term]) -> List[Term]:
    if not terms:
        return []
    terms = sorted(terms, key=functools.cmp_to_key(cmp), reverse=True)
    best = terms[0]
    return list(itertools.takewhile(lambda i: not best.is_better_than(i), terms))

print(get_best_terms(map(Term, [1, 2, 3, 4, 5])))
print(get_best_terms(map(Term, [1, 2, 3, 4])))
print(get_best_terms(map(Term, [5, 1, 2, 4, 3])))
print(get_best_terms(map(Term, [5, 1, 4, 3])))

This can however be simplified if Terms are comparable, as you won't need to use key and can remove the need for a lambda.

@functools.total_ordering
class Term(int):
    def __gt__(self, other: Term) -> bool:
        return self // 2 > other // 2

    def __eq__(self, other: Term) -> bool:
        return self // 2 == other // 2

def get_best_terms(terms: Set[Term]) -> List[Term]:
    if not terms:
        return []
    terms = sorted(terms, reverse=True)
    return list(itertools.takewhile(terms[0].__eq__, terms))


Loading config file from XML After my question on StackOverflow, I've written code that should create a Config class from an XML file. XML loading is mixed with structures declaration and break in a lot of small pieces.

However, I'm not sure if this is bad or good. How would you refactor this? Probably some simple design pattern can be used, but again, I'm not sure that I should use design patterns for such simple things as XML loading.

using System;
using System.Net;
using System.Xml;

namespace Fast.Config
{
public class Config
{
    public Config(string configFile)
    {
        Load(configFile);
    }

    public Channel Fond { get; private set; }

    private void Load(string file)
    {
        XmlDocument doc = new XmlDocument();
        doc.Load(file);

        XmlElement fondChannel = doc.SelectNodes("//channel[@id=\"FOND\"]")[0] as XmlElement;
        Fond = new Channel(fondChannel);
     }
}

public class Channel
{
    public Channel(XmlElement channel)
    {
        Load(channel);
    }

    private void Load(XmlElement node)
    {
        var connections = node.SelectNodes(".//connections");
        var ordersIncrementalConnection = (connections[0] as XmlElement).SelectNodes(".//connection[@id=\"OLR\"]");
        OrdersIncremental = new Connection(ordersIncrementalConnection[0] as XmlElement);
    }

    public Connection OrdersIncremental { get; private set; }
}

public class Connection
{
    public Connection(XmlElement connection)
    {
        Load(connection);
    }

    private void Load(XmlElement connection)
    {
        XmlElement feedA =
            connection.SelectNodes(".//feed[@id=\"A\"]")[0] as XmlElement;
        FeedA = new Feed(feedA);

        XmlElement feedB =
            connection.SelectNodes(".//feed[@id=\"B\"]")[0] as XmlElement;
        FeedB = new Feed(feedB);
    }

    public Feed FeedA { get; private set; }
    public Feed FeedB { get; private set; }
}

public class Feed
{
    public Feed(XmlElement feed)
    {
        Load(feed);
    }

    private void Load(XmlElement feed)
    {
    }

}

}

If you choose to manually parse the Xml document for loading only, I would prefer not to use the XmlDocument classes. They have a performance penalty. Instead use the XPathDocument and the XPathNavigator class to navigate trough your document.

    void ReadXml(XmlReader reader)
    {
        XPathDocument document = new XPathDocument( reader );
        XPathNavigator navigator = document.CreateNavigator();

        foreach (XPathNavigator nav in navigator.Select( "/*/*" ))
        {
            string keyValue = nav.GetAttribute( "SomeAttribute", navigator.NamespaceURI );
        }
    }

Second, i would prefer not to manually parse XML at all but create strong-typed classes in your project that you can serialize and deserialize. That makes the usage of you configuration more easy.


Validating username, password, and email in PHP Let's pretend the following:

 <?php
 // Functions to validate/sanitize user input
 function validateUsername() {
    // If accepted, return true, else return false
 }
 function validatePassword() {
    // If accepted, return true, else return false
 }
 function validateEmail() {
    // If accepted, return true, else return false
 }

This is how I handle user input:

// Getting all user input
$values = $_POST['values'];

$error = false;

if (!validateUsername($username) && $error === false) {
    $errorMessage = "Username can't contain special characters";
    $error = true;
}
if (!validatePassword($password) && $error === false) {
    $errorMessage = "Password is'nt secure enought";
    $error = true;
}
if (!validateEmail($email) && $error === false) {
    $errorMessage = "Email is not correctly formatted";
    $error = true;
}

if ($error === true) {
    echo $errorMessage;
} else {

    // Do something

}
?>

But I'm sure there is a better approach. What is the best (or a good) way to handle user input messages/errors?

Also I've read a tutorial (which unfortunately I can't find anymore) where it was recommended to give the users hints and tips about their input, instead of giving them a big bold red warning.

For example to accept not only 1234 AB (Dutch postcodes), but also 1234ab, 1234AB and 1234 ab and let the script convert it to the official notation 1234 AB.

May this below class will help you.  Put this Valiation class in file named as Validation.php file.

<?php
/**
 * This class will provide server side validation for different rules with custom
 * provided message for respective rule.
 *
 * @author: Alankar More.
 */
class Validation
{
    /**
     * Posted values by the user
     *
     * @var array
     */
    protected static $_values;

    /**
     * Rules set for validation
     *
     * @var array
     */
    protected static $_rules;

    /**
     * Error messages
     *
     * @var array
     */
    protected static $_messages;

    /**
     * To send response
     *
     * @var array
     */
    protected static $_response = array();

    /**
     * For storing HTMl objects
     *
     * @var array
     */
    protected static $_elements;

    /**
     * Html object
     *
     * @var string
     */
    protected static $_inputElement;

    /**
     * Value of Html object
     *
     * @var mixed (string|boolean|integer|double|float)
     */
    protected static $_elementValue;

    /**
     * Name of validation rule
     *
     * @var string
     */
    protected static $_validationRule;

    /**
     * Value of validation rule
     *
     * @var mixed (string|boolean|integer|double|float)
     */
    protected static $_ruleValue;

    /**
     * Initializing class
     *
     * @param array $inputArray
     * @param array $values
     */
    public static function _initialize(array $inputArray, array $values) {
        self::$_values = $values;
        self::$_response = array();

        self::generateArrays($inputArray);
        return self::applyValidation();
    }

    /**
     * Separating rules and values
     *
     * @param array $input
     */
    public static function generateArrays(array $input) {
        self::$_messages = $input['messages'];
        self::$_rules = $input['rules'];
    }

    /**
     * Applying validation for the form values
     *
     */
    public static function applyValidation() {
        foreach (self::$_rules as $rk => $rv) {
            $_element = self::$_rules[$rk];
            if (is_array($_element)) {
                foreach ($_element as $key => $ruleValue) {
                    if (!self::$_elements[$rk]['inValid']) {
                        $method = "_" . $key;
                        self::$_inputElement = $rk;
                        self::$_elementValue = self::$_values[$rk];
                        self::$_validationRule = $key;
                        self::$_ruleValue = $ruleValue;

                        self::$method();
                    }
                }
            }
        }

        if (count(self::$_response) == 0) {
            self::$_response['valid'] = true;
        }

        return self::$_response;
    }

    /**
     * Method to check wheather the input element holds the value.
     * If not then assingn message which is set by the user.
     *
     */
    protected static function _required() {
        if (self::$_ruleValue) {
            if (trim(self::$_elementValue) == NULL &&
                strlen(self::$_elementValue) == 0) {
                self::setErrorMessage("Field Required");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Maximum length of input
     *
     */
    protected static function _maxLength() {
        if (self::$_ruleValue) {
            if (strlen(trim(self::$_elementValue)) > self::$_ruleValue) {
                self::setErrorMessage("Enter at most " . self::$_ruleValue . " charachters only");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Minimum length of input
     *
     */
    protected static function _minLength() {
        if (self::$_ruleValue) {
            if (self::$_ruleValue > strlen(trim(self::$_elementValue))) {
                self::setErrorMessage("Enter at least " . self::$_ruleValue . " charachters ");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Allow alphabets only
     *
     */
    protected static function _number() {
        if (self::$_ruleValue) {
            $str = filter_var(trim(self::$_elementValue), FILTER_SANITIZE_NUMBER_INT);
            if (!preg_match('/[0-9]/', $str)) {
                self:: setErrorMessage("Enter numbers only");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Allow alphabets only
     *
     */
    protected static function _alphabetsOnly() {
        if (self::$_ruleValue) {
            $str = filter_var(trim(self::$_elementValue), FILTER_SANITIZE_STRING);
            if (!preg_match('/[a-zA-z]/', $str)) {
                self:: setErrorMessage("Enter alphabates only");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Allow alphabets and numbers only
     *
     */
    protected static function _alphaNumeric(){
        if (self::$_ruleValue) {
            $str = trim(self::$_elementValue);
            if (!preg_match('/[a-zA-z0-9]/', $str)) {
                self:: setErrorMessage("Alphanumeric only");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * To check enter email is valid
     *
     */
    protected static function _email(){
       if (self::$_ruleValue) {
            $str = filter_var(trim(self::$_elementValue), FILTER_VALIDATE_EMAIL);
            if (!$str) {
                self:: setErrorMessage("Enter valid email");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * To check enter url is valid
     *
     */
    protected static function _url(){
       if (self::$_ruleValue) {
            $str = filter_var(trim(self::$_elementValue), FILTER_VALIDATE_URL);
            if (!$str) {
                self:: setErrorMessage("Enter valid URL");
                self::setInvalidFlag(true);
            } else {
                self::setInvalidFlag(false);
            }
        }
    }

    /**
     * Setting invalid flag for every element
     *
     * @param boolean $flag
     */
    private static function setInvalidFlag($flag) {
        self::$_elements[self::$_inputElement]['inValid'] = $flag;
    }

    /**
     * Setting error message for the input element
     *
     * @param string $message
     */
    private static function setErrorMessage($message) {
        if (self::$_messages[self::$_inputElement][self::$_validationRule]) {
            $message = self::$_messages[self::$_inputElement][self::$_validationRule];
        }
       array_push(self::$_response, ucfirst($message));
    }
}

You can use this class in your application as below:

<form name="frmTest" id="frmTest" action="" method="POST">
    <input type="text" name="first_name" id="first_name" value = "" />
    <button name="submit" value="Submit" type="submit" >Submit</button>
</form>

<?php
require_once 'validation.php';
// Rules specification.
$rules = array('method' => 'POST',
    'rules' => array('first_name' => array('required' => true)
    ),
    'messages' => array('first_name' => array('required' => 'Please enter first name')
    )
);

$userPostedData = $_POST;
$response = Validation::_initialize($rules, $userPostedData);

// if some error messages are present.
if (!$response['valid']) {
    // it will give you the array with error messages.
    echo "<pre>";
    print_r($response);
} else {
    // all applied validations are passed. You can deal with your submitted information now.
    echo "<pre>";
    print_r($_POST);
}
?>


Improvement for std::map  double dispatch I am trying to do my c++ homework and as a part of it I have to implement a std::map with an int as its key and a value should be a specific void function, which prints some text.
I can`t find an answer to what are the correct ways of doing that.

This is what I have so far:

#include <map>
#include <string>
#include <iostream>

class testClass
{
public:
    void testFunc1() { std::cout << "func1\n"; }
    void testFunc2() { std::cout << "func2\n"; }
};

typedef void(testClass::*runFunc)(void);
typedef std::map<int, runFunc> myMapType;

int main() {
    testClass t1;

    std::map<int, runFunc> myMap;
    myMap.emplace(1, &testClass::testFunc1);
    myMap.emplace(2, &testClass::testFunc2);

    myMapType::const_iterator itr;
    itr = myMap.find(2);

    if (itr != myMap.end()) {
        (t1.*(itr->second))();
    }
}

The code is working now, however I am not sure if that is the way people with more experience do.

Also, if you are aware of any sources where one can read more about knowledge required to achieve the needed result - any ideas are appreciated.

In general your code looks good. But I have some little annotations (improvements) for you.

1. I'd prefer direct initialization of itr for sake of readability:

myMapType::const_iterator itr = myMap.find(2);

instead of

myMapType::const_iterator itr;
itr = myMap.find(2);

That said you could also avoid

typedef std::map<int, runFunc> myMapType;

completely using the auto keyword:

auto itr = myMap.find(2);

2. Make use std::function and lambda bindings

For the callable value parameter of the std::map I'd prefer to use an appropriate std::function value type and lambda function bindings to the instance to operate on.

testClass t1;

std::map<int, std::function<void()>> myMap;
myMap.emplace(1, [&t1](){t1.testFunc1();});
myMap.emplace(2, [&t1](){t1.testFunc2();});

This will give you greater flexibility for later changes and use of other classes than just testClass. Also you can get rid of that other type definition then:

typedef void(testClass::*runFunc)(void);

As soon you want to use your map for the general case you can clearly see the benefits:

class A {
public:
     void print() { std::cout << "print() from class A.\n"; }
};

class B {
public:
     void print() { std::cout << "print() from class B.\n"; }
};

// ...
int main() {
    A a;
    B b;

    std::map<int, std::function<void()>> myMap;
    myMap.emplace(1, [&a](){a.print();});
    myMap.emplace(2, [&b](){b.print();});

}

Here's my set of changes in whole (you can check it's still working as intended here):

#include <map>
#include <string>
#include <iostream>
#include <functional>

class testClass
{
public:
    void testFunc1() { std::cout << "func1\n"; }
    void testFunc2() { std::cout << "func2\n"; }
};

int main() {
    testClass t1;

    std::map<int, std::function<void()>> myMap;
    myMap.emplace(1, [&t1](){t1.testFunc1();});
    myMap.emplace(2, [&t1](){t1.testFunc2();});

    auto itr = myMap.find(2);

    if (itr != myMap.end()) {
        (itr->second)();
    }
}


Printing a pattern through the use of a dictionary We had to port a c program to python.  The code works but uses a dictionary to solve the problem.  This doesn't feel like the right use of a dictionary.  Is there a better way?

#!/usr/bin/env python3

def main():
    while True:
        height = input("Height: ")
        try:
            height = int(height)
            if height >= 0:
                break
        except:
            pass

    mtn={}
    space_mtn(height,mtn)
    hashs_mtn(height,mtn)
    print_mtn(height,mtn)

def space_mtn(height,mtn):

    for i in range(height):
        for j in range(2*height+2):
            mtn[i,j]=" "
    return

def hashs_mtn(height,mtn):

    for i in range(height):
        for  j in range(height-1,height-i-2,-1):
            mtn[i,j] = '#'

    for i in range(height):
        for j in range(height+2,height+i+3):
            mtn[i,j] = '#'
    return

def print_mtn(height,mtn):

    for i in range(height):
        for j in range(height+3+i):
            print("{}".format(mtn[i,j]),end="")
        print("")
    return

if __name__ == "__main__":
    main()

For how you're using it, it seems like a list of lists would make more sense than a dict. This is somewhat like a 2-dimensional array in C.

Your mtn initialization becomes:

mtn = []

You'll need to do a little more work in space_mtn to generate the sublists:

def space_mtn(height,mtn):
    for i in range(height):
        mtn.append([])             # Append an empty sublist at each level
        for j in range(2*height+2):
            mtn[-1].append(" ")    # Append a space to the end of the currently-last list
    return

Now in hash_mtn and print_mtn, just change your mtn[i,j] calls to mtn[i][j].

Now, there are lots of ways to make this code less C-like and more "Pythonic". Such discussions are probably better for Code Review. I'll just say that it's possible (although not necessarily "best") to get equivalent functionality by replacing everything after your input while loop with one line:

print('\n'.join('{0:>{1}}  {0:{1}}'.format('#'*(i+1), height) for i in range(height)))


Should I create an abstract runnable? I've 4 sub-classes which perform similar work but only 3 of them have identical constructors I've added the code for an abstract thread class and 2 sub-classes. The structure and job of the threads (sub-classes) is identical except one difference: ONE of the sub-classes, RefreshData does not need the client parameter. I've overloaded the constructor in the sub-class but I'm not sure if the abstraction makes sense anymore. Moreover, can I do something to ensure that RefreshData doesn't use the inherited client parameter accidentally and ends up getting an NPE.

    public abstract class BackgroundTask implements Runnable {

      protected final Manager manager;
      protected final Client client;
      private long msSyncInterval;
      private boolean shutdown; // has a setter

    public BackgroundTask(final Manager manager, final long msSyncInterval) {
        this.manager = manager;
        this.client = null;
        this.msSyncInterval = msSyncInterval;
        this.shutdown = shutdown;
    }

    public BackgroundTask(final Manager manager, final Client client, final long msSyncInterval) {
        this.manager = manager;
        this.client = client;
        this.msSyncInterval = msSyncInterval;
        this.shutdown = shutdown;
    }

    @Override
    public void run() {
        while (!shutdown) {
            executeTask();

            // sleep for msSyncInterval
        }
    }

    abstract void executeTask();
}

public class ExchangeData extends BackgroundTask {

    public ExchangeData(final Manager manager, final Client client, final long msSyncInterval) {
        super(manager, client, msSyncInterval);
    }

    @Override
    void executeTask() {
        // Some work here
    }
}

public class RefreshData extends BackgroundTask {

    public RefreshThrottleLimits(final Manager manager, final long msSyncInterval) {
        super(throttleManager, msSyncInterval);
    }

    @Override
    void executeTask() {
        // Doesn't use the client
    }
}

You have field shutdown that you mutating in a multithreaded code, this field can be cached and might ignore you changing it. You can declare it as volatile to avoid this behavior.

private volatile boolean shutdown;

What makes your abstraction leaky is the fact that BackgroundTask is not abstract enough, that is, Manager and client shouldn't be there.

 public abstract class BackgroundTask implements Runnable {

   abstract void executeTask();
   private boolean shutdown;

   @Override
   public void run() {
    while (!shutdown) {
        executeTask();

        // sleep for msSyncInterval
    }
  }
 }

And now we can have an abstract class that does Task involving clients and managers

 abstract class AbstractManagerClientTask extends BackgroundTask { // not sure about the name
   protected final Manager manager;
   protected final Client client;
   .....
 }

ExchangeData is more specific than a BackgroundTask, it is a AbstractManagerClientTask

  class ExchangeData extends AbstractManagerClientTask{
   ...
  }

It's much easier now, RefreshData can extend BackgroundTask with its own Manager field

 public class RefreshData extends BackgroundTask {
  private final Manager manager;
  ....
 }


Group counter for ranges of values I have this grouping thing, which is would be a switch case if it weren't for the ranges or a Counter of some sorts, but as there are those ranges I don't know how to implement this any more efficiently.

def getGroups(user_array):
    # group1 is group 1-10 x
    group1 = 0
    # group2 is group 10-50 x
    group2 = 0
    # group3 is group 50-100 x
    group3 = 0
    # group4 is group 100-200 x
    group4 = 0
    # group5 is group 200-500 x
    group5 = 0
    # group6 is group 500 - 1000 x
    group6 = 0
    # group7 is group 1000+ x
    group7 = 0

    for user in user_array:
        if user.x_count == 0:
            pass
        elif user.x_count <= 10:
            group1 += 1
        elif user.x_count <= 50:
            group2 += 1
        elif user.x_count <= 100:
            group3 += 1
        elif user.x_count <= 200:
            group4 += 1
        elif user.x_count <= 500:
            group5 += 1
        elif user.x_count <= 1000:
            group6 += 1
        else:
            group7 += 1
    return [group1, group2, group3, group4, group5, group6, group7]

If your data is strictly integer values, you can use user.x_count in range(...) to test whether or not the user.x_count value is a member of the range(...) set.  Ie)

def getUsers(user_array):
    group1 = sum(1 for user in user_array if user.x_count in range(1, 11))
    group2 = sum(1 for user in user_array if user.x_count in range(11, 51))
    # ... etc ...

This unfortunately will require several passes through your user_array data, so will not work if that data is ephemeral, such as iterator or generator based.

A more complex method will categorize the user.x_count value into a group value, and then increment the appropriate group counter.  bisect will find an insertion index in a sorted array, so we can leverage this to turn a user.x_count into a group based on its corresponding insertion index.  This will function properly if floating point values are encountered.

import bisect

def getUsers(user_array):
    thresholds = (0, 10, 50, 100, 200, 500, 1000)
    groups = [0] * (len(thresholds) + 1)

    for user in user_array:
        groups[bisect.bisect_left(thresholds, user.x_count)] += 1

    return groups[1:]

Notice there are no more group-specific variables, like group1.  Instead, all counters are created based on data, allowing you to add additional groups without modifying lines of code; you just modify data.


Assembly Summation Implementation I wrote a simple summation program in MARS to practice using loops and jump instructions. I feel like this is pretty lightweight and wanted some feedback on possible improvements:

Lowering instruction count
Organizational changes

Am I correct in assuming this is a "do-while" loop?

# Description: Summation program implementing a do-while loop
# Author: Evan Bechtol

        .data
Prompt: .asciiz "Enter number to get summation: "
Output: .ascii  "Sum of numbers is: "
        .globl main
        .text
main:
    # Prompt for input
    li   $v0, 4
    la   $a0, Prompt
    syscall

    # Read input
    li   $v0, 5
    syscall
    add  $s0, $zero, $v0

    # Begin summation
    addi $s1, $zero, 1 # $s1 used to store summation, starts at 1
    addi $t1, $zero, 1 # $t1 used as counter, starts at 1
    jal  loop          # Go to loop

    # Display
    li   $v0, 4
    la   $a0, Output
    syscall

    li   $v0, 1
    add  $a0, $zero, $s1
    syscall

    # Exit
    la   $v0, 10
    syscall

loop:
    addi $t1, $t1, 1
    add  $s1, $s1, $t1
    bne  $t1, $s0, loop
    jr   $ra

I see a number of things that might help you improve your program.

Use li rather than la to load a constant

As has already been noted, the la $v0, 10 instruction should actually be li $v0, 10.  It works that way because you're loading the address 10 into $v0 which has the same effect, but it's a little strange looking.

Put the $zero at the end of the instruction

The instruction add  $s0, $zero, $v0 is perfectly valid, but it might look a little odd to those experienced with MIPS assembly language.  The typical idiom that I've seen is to put the constant at the end, so that instruction would instead read add  $s0, $v0, $zero which does exactly the same thing, but I find it easier to pick out the $zero at the end rather than in the middle.  That's purely a stylistic suggestion, though.

Minimize register use

You don't really need both a counter and a number.  What you can do instead is to use the original input number in $s0 as the counter and decrement instead of incrementing.

Avoid the overhead of subroutine calls

There's nothing wrong with using subroutines, of course, but they add cycles to the program, and for such a small and simple program as this, the call isn't really necessary.  Instead, you could put the loop inline.

Prefer to branch at the end of a loop

A while... loop in higher level languages does not even execute once if its condition is false.  A do ... while loop executes at least once, which seems to me to be what is desired here.  You can then minimize branching so that there is only one conditional branch at the end of the loop.

Use a better algorithm

Carl Friedrich Gauss came up with a clever algorithm as a child.  You can read about it and see if you can implement it for even faster code.

Putting it all together

When we apply all but the last suggestion, we get something like this:

        .data
Prompt: .asciiz "Enter number to get summation: "
Output: .ascii  "Sum of numbers is: "
        .globl main
        .text
main:
    # Prompt for input
    li   $v0, 4
    la   $a0, Prompt
    syscall

    # Read input
    li   $v0, 5
    syscall
    add  $s0, $v0, $zero  # s0 used as loop countdown
    add  $s1, $s1, $zero  # s1 used for summation = 0
loop:
    add  $s1, $s1, $s0    # add number to summation

    # Begin summation
    addi $s0, $s0, -1     # decrement number
    bgtz $s0, loop        # keep going until 0

        # Display
    li   $v0, 4
    la   $a0, Output
    syscall

    li   $v0, 1
    add  $a0, $s1, $zero
    syscall

    # Exit
    li   $v0, 10
    syscall


Maximum of three values in C++ When I need to get maximum value of A, B, and C, then I would write like:

val = A;
val = max(val, B);
val = max(val, C);

But, in this case, I need to write two "val" in the one line. And, it is cumbersome if the "val" is more complex like "val[0][1][2].element"... If it is adding value, I can write like "val += B" without writing "val" twice in the line.

I can make macro like:

#define MAXEQ(A,B) (A) = max((A), (B))

But, it looks not very intuitive.

Is there any smarter way to write these things ?

If you have access to C++11, I'd suggest using std::max with an initializer_list<T>. A complete minimal example:

#include <algorithm>
#include <initializer_list>
#include <iostream>

int main()
{
    int a = 1;
    int b = 2;
    int c = 3;

    int m = std::max({a, b, c});

    std::cout << m << "\n";
}


Test for Array vs Range in my TEXTJOIN UDF I use the following code in many of my answers on Stack Overflow, to mimic the new TEXTJOIN function available in Office 365 Excel:

Function TEXTJOIN(delim As String, skipblank As Boolean, arr)
    Dim d As Long
    Dim c As Long
    Dim arr2()
    Dim t As Long, y As Long
    t = -1
    y = -1
    If TypeName(arr) = "Range" Then
        arr2 = arr.Value
    Else
        arr2 = arr
    End If
    On Error Resume Next
    t = UBound(arr2, 2)
    y = UBound(arr2, 1)
    On Error GoTo 0

    If t >= 0 And y >= 0 Then
        For c = LBound(arr2, 1) To UBound(arr2, 1)
            For d = LBound(arr2, 1) To UBound(arr2, 2)
                If arr2(c, d) <> "" Or Not skipblank Then
                    TEXTJOIN = TEXTJOIN & arr2(c, d) & delim
                End If
            Next d
        Next c
    Else
        For c = LBound(arr2) To UBound(arr2)
            If arr2(c) <> "" Or Not skipblank Then
                TEXTJOIN = TEXTJOIN & arr2(c) & delim
            End If
        Next c
    End If
    TEXTJOIN = Left(TEXTJOIN, Len(TEXTJOIN) - Len(delim))
End Function

I have successfully made it so it works with both ranges and arrays, so it works with:

=TEXTJOIN(",",TRUE,A1:B7)

As well as

{=TEXTJOIN(",",TRUE,IF(A1:A7 = "x",B1:B7,""))}

The question is:  Is this the best method to check for array vs. range?  Is there a better method?

As stated this works, nicely with both, but I have to think that I am doing it in a round about manner.

Okay, let's first eliminate the easy stuff Rubberduck points out:

Function is implicitly Public and implicitly returns a Variant - should be a String.
All parameters are implicitly passed ByRef (and semantically should be ByVal).
Parameter arr is implicitly Variant,
Local variables c, d, t, y, arr2 all have terrible, meaningless names (arr, matey! ..I like calling it Pirate Notation ;)
String-returning Left$ function should be used over Variant-returning Left function.
vbNullString should probably be preferred over "" empty string literal.

With default settings Rubberduck will also complain about Dim t As Long, y As Long, because having multiple declarations in a single instruction isn't ideal.

The variables are declared at the top of the function rather than as close as possible to their usage, which makes it hard to see what's used where.

Type-checking isn't type safe:

If TypeName(arr) = "Range" Then
    arr2 = arr.Value
Else
    arr2 = arr
End If

If the Microsoft Word object model is referenced, you can pass a Word.Range object and it will happily take it; same if I made my own Range class and gave it an instance. Use the TypeOf operator to perform compile-time type-safe type checks:

If TypeOf arr Is Excel.Range Then
    'definitely an Excel Range object
Else
    'could be anything
End If

Note, in the Else branch arr could literally be anything - but you're assuming it's an array. You could use the IsArray function to make sure of that, and then you can also assert on the upper bound using a rather ugly helper function, to make sure you're looking at the 2D array you're expecting.

Else
    Debug.Assert IsArray(arr)
    Debug.Assert GetArrayDimSize(arr) = 2
    arr2 = arr
End If

With the array dimension count known/asserted, the On Error Resume Next statement that follows can be removed... and t can be renamed sourceColumns or colCount or whatever, and y can be renamed sourceRows or rowCount or whatever - and arr2 could be renamed to sourceArray:

sourceColumns = UBound(sourceArray, 2)
sourceRows = UBound(sourceArray, 1)

And with that naming scheme I think I'd rename the arr parameter to source.

If t >= 0 And y >= 0 Then

Ah, ok, so that condition is leveraging the fact that VBA would have thrown an error given a 1D array, leaving t = 0. Not very obvious, let's improve that.

But first we need to tweak the assertions - we don't really want a 2D array, we just want any array with at most two dimensions. So... let's be less forgiving given anything other than that:

Dim dimensionCount As Long
If TypeOf(arr) Is Excel.Range Then
    sourceArray = source.Value
    dimensionCount = 2 'Range.Value is always a 2D array
Else
    Dim isValidArray As Boolean
    isValidArray = IsArray(source)
    If isValidArray Then dimensionCount = GetArrayDimSize(source)
    isValidArray = dimensionCount <> 0 And dimensionCount <= 2
    If Not isValidArray Then Err.Raise 5, "TEXTJOIN", "Expected: 1D or 2D array"
End If

And now that If statement can be much more explicit about what's going on and why:

If dimensionCount = 2 Then
    'handle 2D array
Else
    Debug.Assert dimensionCount = 1
    'handle 1D array
End If

So, c iterates dimension 1 / rows, d iterates dimension 2 / columns.

Now all these concatenations are inherently slow. I realize that's "just a worksheet function" and you'd like to share it as a single, simple, cohesive and focused little piece of code... but given thousands of iterations, VBA's string-handling will start becoming the performance bottleneck of the function.

There's a lightning-fast StringBuilder class right here on this site that you can use to address that.

As a bonus, using a StringBuilder makes the function assign to its return value only once - as opposed to what you have now, which is treating the return-value-identifier as if it were a local variable (which it technically is, ...I just don't like doing that).

Not sure why the d / column loop is inconsistent here:

For d = LBound(arr2, 1) To UBound(arr2, 2)

You're iterating the 2nd dimension, the LBound should be off the 2nd dimension too (yes, it should be the very same as that of the 1st dimension). Also, the upper bounds of both dimensions are already known and stored in local variables:

For currentRow = LBound(sourceArray, 1) To sourceRows
    For currentColumn = LBound(sourceArray, 2) To sourceColumns

This code will throw an error (by design?) if the array contains an Error value:

If arr2(c, d) <> "" Or Not skipblank Then

You could have a parameter that helps you decide how to treat errors - perhaps an Enum could be used:

Public Enum TEXTJOIN_ErrorValues
    ThrowOnError
    SkipError
    IncludeErrorText
End Enum

ThrowOnError would be the current/default behavior; SkipError would treat errors as blanks, and IncludeErrorText would include the e.g. #N/A error text into the result.

Except, once you have an Variant/Error value and not a Range, it's pretty much impossible to get the text back (unless you want to map CVErr(xlErrWhatever) values to a corresponding string... probably not worth it) - so scratch that enum value, and the behavior becomes either throw on error, or skip error values. And that's entirely possible, and quite easy to do - an additional optional Boolean parameter could be helpful for that.

Dim sb As StringBuilder
Set sb = New StringBuilder
'...

For currentRow = LBound(sourceArray, 1) To sourceRows
    For currentColumn = LBound(sourceArray, 2) To sourceColumns
        If Not IsError(sourceArray(currentRow, currentColumn)) Then
            If sourceArray(currentRow, currentColumn) <> vbNullString Or Not skipBlank Then
                sb.Append sourceArray(currentRow, currentColumn)
                sb.Append delim
            End If
        ElseIf Not skipErrors Then
            sb.Append delim
        End If
    Next
Next

TEXTJOIN = sb.ToString

Now, that's the 2D loop.. the 1D loop does essentially the same thing.. and that's annoying. I'd extract a method for that.

Private Sub ProcessValue(ByVal value As Variant, ByVal sb As StringBuilder, ByVal delim As String, ByVal skipBlanks As Boolean, ByVal skipErrors As Boolean)
    If Not IsError(value) Then
        If CStr(value) <> vbNullString Or Not skipBlanks Then
            sb.Append CStr(value)
            sb.Append delim
        End If
    ElseIf Not skipErrors Then
        sb.Append delim
    End If
End Sub

That turns the logic into:

If dimensionCount = 2 Then
    For currentRow = LBound(sourceArray, 1) To sourceRows
        For currentColumn = LBound(sourceArray, 2) To sourceColumns
            ProcessValue sourceArray(currentRow, currentColumn), sb, delim, skipBlanks, skipErrors
        Next
    Next
Else
    Debug.Assert dimensionCount = 1
    For currentRow = LBound(sourceArray, 1) To sourceRows
        ProcessValue sourceArray(currentRow), sb, delim, skipBlanks, skipErrors
    Next
End If

TEXTJOIN = sb.ToString

The order of the parameters strikes me as unintuitive - might be by design to match Microsoft's function, by I would have made the source array/range the first parameter, followed by an optional delimiter, followed by an optional flag to skip blanks (followed by an optional flag to skip errors).


Compute Gini Coefficient Recently, I was given a math assignment to calculate Gini Indexes for a table of percent distributions of aggregate income.

The table takes the form of:

Year    1st Quintile    2nd Quintile    3rd Quintile    4th Quintile    5th Quintile
----    ------------    ------------    ------------    ------------    ------------
1929        0.03             12.47           13.8           19.3              54.4
1935        4.1              9.2             14.1           20.9              51.7
...         ...              ...             ...            ...               ...

Because of the length of the actual table I wrote a short python script to calculate the Gini Indexes. However, I'm fairly new to Python so I'd like to see what needs improvement.

Method of Calculation:

Calculate the log of the percentages with the quintile as the base of the logarithm (i.e. log(0.0003)/log(0.2), log(0.1247)/log(0.4) ...) and then average these values to find an approximate exponent for the Lorenz curve. Calculate the Gini Index by finding twice the area between y=x and the Lorenz curve from 0 to 1.

import numpy as np
import scipy.integrate as integrate
import itertools as it

def log_slope(x, y):
    return np.log(y)/np.log(x)

# read in data from file
years, data = read_values('Quintiles')  # shape: [[0.03, 0.12,...], [], []..., []]
accumulated_vals = [list(it.accumulate(v)) for v in data]

# percentiles; remove 0.0 and 1.0 after calculating
percentiles = np.linspace(0.0, 1.0, np.shape(accumulated_vals)[1] + 1)[1:-1]

for j, vals in enumerate(accumulated_vals):
    sum = 0
    for i, val in enumerate(vals[:-1]):  # exclude the last accumulated value, which should be 1.0
        sum += log_slope(percentiles[i], val)
    average = sum / (len(vals)-1)
    gini = 2 * integrate.quad(lambda x: x - pow(x, average), 0.0, 1.0)[0]
    print('{:d}: {} -> {:.5f}'.format(int(years[j]), [round(k, 4) for k in vals], gini))

Questions:

The code produces correct values and is already pretty quick, but can its speed be improved? At the moment I'd like to keep the method of calculation the same because it's the method used in my calculus course, but are there any possible improvements to this method?
Can the clarity of the code be improved? Are the variable names clear?
Does the code follow standard python conventions? I'm new to Python so I'm not familiar with these.
Minor Question/Bug: Printing the accumulated values will sometimes result in the last value being 1.00001 or 0.999 rather than 1.0. I presume this is due to precision errors, but it's seems odd when dealing with adding numbers with only 3 decimal places. Is there an easy fix for this?

Stick closely to the sources

It's helpful when coding math in cases like this to base your approach on established methods and language.

It might seem a bit extreme, but this can include:

Following a published method to the letter.
Linking to a readily-available description of it.
Naming your variables and laying out your code as closely as possible to that description.

eg.

def gini_index(*args*):
    """
    Calculates the Gini Index G given data of the form:
    *whatever form your data is*
    Using summation as described in:
    *reference* (can be textbook, arxiv etc)
    Via the formula:
    *include formulae if possible*
    Essentially as described here:
    https://en.wikipedia.org/wiki/Gini_coefficient#Alternate_expressions
    Example input:
    *your example input*
    Example output:
    *your example output*
    """
#   your code proper starts here

An example of this is here, a  collection of basic number theory and elliptic curve utilities that eventually became SageMath.

For example, were you actually to use this formula as a basis, you would name your counter i, your total number of values n, and use the summation functions provided in numpy.

Trust me, if you think you'll revisit your code at a later date (when memory fails), or have it used or modified by someone else, this is a lifesaver.

(Edit: As you are integrating a polynomial, you can use the integ method of poly1d included with numpy rather than importing scipy.)


Menu script efficiency I've got a jQuery/JavaScript script for a menu that uses icons as well as label text. When the icon or text is clicked, it turns blue, until such time as another icon is clicked.
Also, the page scrolls down to the relevant div on the page, but the menu bar still stays fixed along the top of the screen.

I've created several versions of this script and this is the most efficient (least code) I have managed so far.

Can anyone tell me any methods to use instead, or any other way I could make it more efficient?

window.onload = function() {

        [].forEach.call(document.querySelectorAll('.navico'), function(el) {
        el.addEventListener('click', imageButtonClickHandler);
    });

        function imageButtonClickHandler()
        {
                switch(this.id)
                {
                        case "aboutnav":
                                grey();
                                $('#abouttitle').css('color', 'blue');
                                $('#a').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#about").offset().top - 90 }, 2000);
                        break;
                        case "routenav":
                                grey();
                                $('#routetitle').css('color', 'blue');
                                $('#b').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#route").offset().top - 90 }, 2000);
                        break;
                        case "enternav":
                                grey();
                                $('#entertitle').css('color', 'blue');
                                $('#c').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#enter").offset().top - 90 }, 2000);
                        break;
                        case "racedaynav":
                                grey();
                                $('#racedaytitle').css('color', 'blue');
                                $('#d').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#raceday").offset().top - 90 }, 2000);
                        break;
                        case "gallerynav":
                                grey();
                                $('#gallerytitle').css('color', 'blue');
                                $('#e').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#gallery").offset().top - 90 }, 2000);
                        break;
                        case "newsnav":
                                grey();
                                $('#newstitle').css('color', 'blue');
                                $('#f').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#news").offset().top - 90 }, 2000);
                        break;
                        case "resultsnav":
                                grey();
                                $('#resultstitle').css('color', 'blue');
                                $('#g').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#resu").offset().top - 90 }, 2000);
                        break;
                        case "contactnav":
                                grey();
                                $('#contacttitle').css('color', 'blue');
                                $('#h').css('color', 'blue');
                                $('html, body').animate( { scrollTop: $("#contact").offset().top - 90 }, 2000);
                        break;

                }
        }
};

function grey()
        {
                $('.sectitle').css('color', 'grey');
                $('.roger').css('color', 'grey');
                return;
}

I am happy you posted your best script, you can still see a ton of repetition here.. If you were to always name the navbutton after the scroll target and title ( "aboutnav" -> about & abouttitle ) then this can be much shorter. For now I can only see resultsnav break the mold.

I would suggest something like this :

window.onload = function () {

    [].forEach.call(document.querySelectorAll('.navico'), function (el) {
        el.addEventListener('click', imageButtonClickHandler);
    });

    function imageButtonClickHandler() {
      var buttonId = this.id,
          target   = buttonId.substring( 0 , buttonId.length - 3 ),
          titleId = '#' + target + 'title',
          scrollTarget = '#' + target;

      var navMap = {
          'aboutnav'   : '#a',
          'routenav'   : '#b',
          'enternav'   : '#c',
          'racedaynav' : '#d'
          //etc. etc.
      }

      grey();
      $( titleId ).css('color', 'blue');
      $( navMap[ target ] ).css('color', 'blue');
      $('html, body').animate({
        scrollTop: $(scrollTarget).offset().top - 90
      }, 2000);
   }
};

function grey() {
    $('.sectitle').css('color', 'grey');
    $('.roger').css('color', 'grey');
    return;
}

Basically, I derive the link id, the scroll target id from the name of the nav link. Then, I also mapped the nav names to 'a','b','c' etc. From there this becomes a really short piece of code. I also changed the tabs to 4 spaces, if you use more than 4 spaces, then it becomes harder to grok the code and realy understand what you are doing. I also opted for single quote strings, easier to read and less carpal tunnel by hitting shift all the time ;)

Note that you could write grey like this:

function grey() {
    $('.sectitle, .roger').css('color', 'grey');
}

You can comma chain selectors, also the function will return regardless at the end, you dont need to put return.


Rotate a N by N matrix by 90 degrees clockwise The task:

  Given an N by N matrix, rotate it by 90 degrees clockwise.

  For example, given the following matrix:

  [[1, 2, 3],  [4, 5, 6],  [7, 8, 9]] you should return:

  [[7, 4, 1],  [8, 5, 2],  [9, 6, 3]]

  Follow-up: What if you couldn't use any extra space?

const mtx = [[1, 2, 3],
 [4, 5, 6],
 [7, 8, 9]];

My imperative solution:

function transpose(matrix){
  const size = matrix[0].length;
  const newMatrix = JSON.parse(JSON.stringify(matrix));
  for (let rowIndex = 0; rowIndex < size; rowIndex++) {
    const row = matrix[rowIndex];
    for (let colIndex = 0; colIndex < size; colIndex++) {
      const newRowIndex = colIndex;
      const newColIndex = size - 1 - rowIndex;
      newMatrix[newRowIndex][newColIndex] = row[colIndex];
    }
  }
  return newMatrix;
};

console.log(transpose(mtx));

My declarative solution:

const transpose2 = matrix => {
  const size = matrix[0].length;
  const newMatrix = JSON.parse(JSON.stringify(matrix));
  matrix.forEach((row, rowIndex) => {
    row.forEach((val, colIndex) => {
      const newRowIndex = colIndex;
      const newColIndex = size - 1 - rowIndex;
      newMatrix[newRowIndex][newColIndex] = val;
    });
  });
  return newMatrix;
};

console.log(transpose2(mtx));

My solution to the Follow Up question:

Didn't really understand the part with "no extra space". The only solution that came to my mind was to overwrite the existing matrix with the new values and at the end clean it up by deleting the old values. ...but appending new values to old values requires extra space, doesn't it?

const transposeNoSpace = matrix => {
  const size = matrix[0].length;
  matrix.forEach((row, rowIndex) => {
    row.forEach((val, colIndex) => {
      const newRowIndex = colIndex;
      const newColIndex = size - 1 - rowIndex;
      const newVal = val[1] ? val[0] : val;
      matrix[newRowIndex][newColIndex] = [matrix[newRowIndex][newColIndex], newVal];
    });
  });
  return matrix.map(row => row.map(col => col[1]));
};

console.log(transposeNoSpace(mtx));

I'd also be interested in a pure functional solution

Copying 2D array

JSON is not for copying data.

const newMatrix = JSON.parse(JSON.stringify(matrix))

Should be

const newMatrix = matrix.map(row => [...row]);

Naming

The naming you use is too verbose and gets in the way of readability.

For 2D data it is common to use row, col or x,y to refer to the coordinates of an item.

As x and y are traditionally ordered with x (column) first and y (row) second 2D arrays don't lend them selves to that naming. It is acceptable to abbreviate to r, c or indexing 2D arrays (within the iterators, not outside)
transpose as a function name is too general. It would be acceptable if transpose included a argument to define how to transpose the array but in this case rotateCW would be more fitting.
For 2D data the distance from one row to the same position on the next is called the stride
As the input array is square there is no need to get the stride from the inner array.  const stride = matrix[0].length; should be   const stride = matrix.length;

Rewriting your solutions

Both these solutions are \$O(n)\$ time and space.

function rotateCW(arr) {
    const stride = arr.length;
    const res = arr.map(row => [...row]);
    for (let r = 0; r < stride; r++) {
        const row = arr[r];
        for (let c = 0; c < stride; c++) {
            res[c][stride - 1 - r] = row[c];
        }
    }
    return res;
};  // << redundant semicolon. Lines end with ; or } not both

Be careful when describing a function as declarative. Though the definition is somewhat loose you should make the effort to declare all high level processing as named functions and the imperative code at the lowest level.

const rotateCW = arr => {
    const rotItem = (r, c, item) => res[r][arr.length - 1 - c] = item;
    const processRow = (row, r) => row.forEach((item, c) => rotItem(c, r, item));
    const res = arr.map(row => [...row]);
    arr.forEach(processRow);
    return res;
}

Functional

Don't get caught up on declarative, in your past questions you had the focus on functional. You should keep that focus.

In this case the core of the solution is converting a row column reference (or coordinate) into the inverse of the rotation. We rotate CW by replacing the current item by the one 90deg CCW of it. arr[y][x] = arr[stride-1-x][y]

The next example is functional and also the smallest code.

const rotateCW = arr => {
    const rotItem = (r, c) => arr[arr.length - 1 - r][c];
    const processRow = (row, r) => row.map((item, c) => rotItem(c, r));
    return arr.map(processRow);
}

or as a one liner

const rotCW = arr => arr.map((row, y) => row.map((v, x) => arr[arr.length - 1 - x][y]));

The \$O(1)\$ space solution.

The "no extra space" simply means that it should be \$O(1)\$ space complexity. This can be done via the traditional swap,

 var a = 1, b = 2;
 const temp = a;
 a = b;
 b = temp;

Or in ES6+

 var a = 1, b = 2;
 [a,b] = [b,a];

The swap does not have to be just two items but can be over as many as you want (shifting), the only requirement is that only one spare slot is needed to shift all items.

var a = 1, b = 2, c = 3, d = 4;
const temp = a;
a = b;
b = c;
c = d;
d = temp;

Or in ES6+

var a = 1, b = 2, c = 3, d = 4;
[a, b, c, d] = [b, c, d, a];

Note The ES6 method for swapping, as a source code complexity reduction, is great...

but the JS engine does not know you are just swapping (or shifting), it creates an array to hold all the items on the right so that it does not overwrite them when assigning the new values. That means that the ES6+ swap is \$O(n)\$ space complexity.

Example

I come from a very heavy visual related background and x,y are the most logical way to index 2D+ arrays so will use it in this example.

function rotate2DArray(arr) {
    const stride = arr.length, end = stride - 1, half = stride / 2 | 0;
    var y = 0;
    while (y < half) {
        let x = y;
        while (x < end - y) {
            const temp = arr[y][x];
            arr[y][x] = arr[end - x][y];
            arr[end - x][y] = arr[end - y][end - x];
            arr[end - y][end - x] = arr[x][end - y];
            arr[x][end - y] = temp;
            x ++;
        }
        y++;
    }
    return arr;
}

The above is very unreadable and it pays to put a little order and alignment to the source

function rotate2DArray(arr) {
    const stride = arr.length, end = stride - 1, half = stride / 2 | 0;
    var y = 0;
    while (y < half) {
        let x = y;
        const ey = end - y;
        while (x < ey) {
            const temp = arr[y][x], ex = end - x;
            arr[y ][x ] = arr[ex][y ];
            arr[ex][y ] = arr[ey][ex];
            arr[ey][ex] = arr[x ][ey];
            arr[x ][ey] = temp;
            x ++;
        }
        y++;
    }
    return arr;
}

Thus the array is rotated in place for \$O(1)\$ space.


Readability of JavaScript shorthand for converting strings to numbers with default value This is perfectly readable code to me, but I'm afraid it may be too confusing to others.  What do you think?

Note that I'm not interested in validation of the input here (validation will happen long before we get to this line in the code), so assume it is blank or a valid non-negative integer.

It's modified from this Stack Overflow question.

var numberToAdd = parseInt($('#number-to-add').val()) || 500; // if a number is not provided, use 500

Seems fine syntactically and perfectly idomatic, but always, always supply a radix argument to parseInt:

var numberToAdd = parseInt($('#number-to-add').val(), 10) || 500;

To quote MDN:

  radix: An integer that represents the radix of the [...] string. Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified.
  [...]
  If the input string begins with "0", radix is eight (octal) or 10 (decimal).  Exactly which radix is chosen is implementation-dependent.  ECMAScript 5 specifies that 10 (decimal) is used, but not all browsers support this yet.  For this reason always specify a radix when using parseInt.

(emphasis in the original)


Get the last activity per user per day in a dataframe I have many users. Each time a user uses their smartphone it register them. I am determining the last time each user used their smartphone each day.
Additionally smartphone usages from 18:00 to 06:00, the next day, should be considered as an entry on the previous day. I have created a dummy example.
I did the following:

First subtract the number of hours.
Sort the data frame based on user and date time.
Get the last row.

Is there a more efficient approach to this? Are there other tips I can follow to improve my code?
df_example = {'id': [1,1,1,1,1],
             'activity': [datetime.datetime(2019, 12, 1, 19, 30, 1),
                         datetime.datetime(2019, 12, 1, 20, 22, 2),
                         datetime.datetime(2019, 12, 2, 2, 13, 2),
                         datetime.datetime(2019, 12, 3, 19, 12, 2),
                         datetime.datetime(2019, 12, 3, 21, 3, 1)
                         ]}
df_example = pd.DataFrame(df_example, columns = ['id', 'activity'])
df_example['activity'] = df_example['activity'] - datetime.timedelta(hours=6, minutes=0)
df_example['date'] = df_example['activity'].apply(lambda x: x.date())
df_example.sort_values(by=['id', 'activity'])
df_example.groupby(['id', 'date']).tail(1)

Instead of using tail, if you only need one item there are the first and last methods, which do exactly what you think they would do with grouped dataframes:
df_example.groupby(['id', 'date']).last()

I doubt there is a faster way to create the activity column. And you have to create a new one because of your requirement with what counts to which day.
But you can speed up getting the date. Using apply with a lambda is about the second slowest way to work with pandas (manual Python for loops are slower). Instead use the vectorized datetime functions:
df_example['date'] = df_example['activity'].dt.date

Potential bug:
Note that df_example.sort_values(by=['id', 'activity']) returns the sorted dataframe, it does not modify it inplace. Either assign it back to df_example, or use inplace=True.
The same is true for groupby, you probably want to assign the result to a variable as well in order to do something else with it afterwards.

Python has an official style-guide, PEP8. While it recommends using spaces around the = operator when using it for assignment, it recommends using no spaces when using it for keyword arguments.


Controller for an Administrator User, can this be improved? (codeigniter) I want to write better code.
This is a simple controller class for an administrator login.

Are there conventions or tips in PHP to rewrite this code and improve it?

<?php

class Administrators extends CI_Controller {

    public function __construct() {
        parent::__construct();
    }

    public function index() {
        if(!$this->session->userdata('logged_in')) {
            redirect('/office/administrators/login');
        } else {
            redirect('/office/dashboard/');
        }
    }

    public function login() {

        if($this->session->userdata('logged_in')) {
            redirect('/office/dashboard/');
        }

        if($_SERVER['REQUEST_METHOD'] == 'POST') {

             $a = new Administrator();

            if($a->login($this->input->post('email'),$this->input->post('password'))) {
                    redirect('/office/dashboard/');
            } else {
                $this->messages->add('Unable to authenticate you', 'error');

            }
        }

        $data['error_messages'] = $this->messages->get('error');
        $this->load->view('/office/administrators/login', $data);
    }

    public function logout() {
        $this->simpleloginsecure->logout();
        redirect('office/administrators/login');
    }

}

?>

the very first characters should be <? to avoid whitespace from being sent in the response. This saves you from issues with header redirects. You should also omit the closing ?> for the same reason (whitespaces).

In order for this function to work it must be used before anything is outputted to the browser since it utilizes server headers.

you can use the input class's post() to check for the post request.

The function (post()) returns FALSE (boolean) if there are no items in the POST.

Here's modified code with comments:
<?php
//should be the very first characters

class Administrators extends CI_Controller {

    public function __construct() {
        parent::__construct();
    }

    //factor out the login check to isolate the common logic
    //also, for understandability, boolean checks should be prefixed
    //with "is" or "has" like phrases
    //like "is it like this?", "[something] has this?"
    private function isLoggedIn(){
       return $this->session->userdata('logged_in');
    }

    public function index() {

        //use our new "isLoggedIn()"
        if(isLoggedIn()) {
            redirect('/office/dashboard/');
        } else {
            redirect('/office/administrators/login');
        }
    }

    public function login() {

        if(isLoggedIn()) {
            redirect('/office/dashboard/');
        }

        //you can use the input class's post() to check for the post request
        if($this->input->post()) {

            //for readability, it's more appropriate to
            //assign these values to verbose variables names...
            $administrator = new Administrator();
            $username = $this->input->post('email');
            $password = $this->input->post('password');

            //...so we know what arguments were sent
            if($administrator->login($username,$password)) {
                redirect('/office/dashboard/');
            } else {
                $this->messages->add('Unable to authenticate you', 'error');
            }
        }

        $data['error_messages'] = $this->messages->get('error');
        $this->load->view('/office/administrators/login', $data);
    }

    public function logout() {
        $this->simpleloginsecure->logout();
        redirect('office/administrators/login');
    }

}

//remove the closing ?>.


Loop with enclosing items every 'n' steps I'm listing elements with a foreach, and I would like to enclose items in something tag n by n:

$i = 0;
$open = $done = TRUE;
$n = 3;
$opening = "<div>";
$closing = "</div>";
$names = array("Doc", "Marty", "George", "Lorraine", "Einstein", "Biff");

foreach($names as $name) { /** $condition was not a bool. My fault. */

    if($open && !($i % n)) {
        print $opening;
        $open = FALSE;
        $done = !$open;
    } elseif(!($i % n)) $done = FALSE;

    /** print element. */
    print $name;

    if(!$open && !($i % n) && !$done) {
        print $closing;
        $open = TRUE;
        $done = !$open;
    }

    $i++;

}

Do you think this snippet can be improved? It would be better to have fewer check variables such as $open or $done.

One problem with your solution is that it won't print the last closing </div> tag if there are less than $n elements inside the last <div>.

An approach which fixes that problem and also simplifies the conditional logic for deciding when to print the tags is to chunk the array first using array_chunk and then iterate over the chunks. Now we only need one boolean variable to remember which chunks to surround in tags.

function alternate($names, $n, $opening, $closing) {
    $tag = TRUE;
    foreach(array_chunk($names, $n) as $chunk) {
        if($tag) {
            print $opening;
        }
        foreach($chunk as $name) {
            print $name;
        }
        if($tag) {
            print $closing;
        }
        $tag = !$tag;
    }
}


Updating CSS for "active" tab I'd like to get some feedback on this very basic JavaScript to update which tab is showing as "active" on my simple web page. This is all in a file titled adjustactive.js and included in my footer template for all pages like so:

<script src="assets/js/adjustactive.js"></script>

Here's the full content of that .js file:

//function courtesy of comments in http://befused.com/javascript/get-filename-url
function getFileName() {
    var url = document.location.href;
    url = url.substring(0, (url.indexOf("#") == -1) ? url.length : url.indexOf("#"));
    url = url.substring(0, (url.indexOf("?") == -1) ? url.length : url.indexOf("?"));
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if(url.length == 0) url = "index";
    return url;
}

function updateNav() {
    var idName = getFileName();
    document.getElementById(idName).className = "active";
}

window.onload = updateNav();

First off, you have a bug. This instruction

window.onload = updateNav();

doesn't execute updateNav on load. It executes updateNav immediately and assigns its return value (which is undefined) to onload. Instead it should be:

window.onload = updateNav;

Also assigning event handlers directly to on... properties isn't something you should be doing. The main problem is you are potentially overwriting any other scripts that also addign to onload. You should instead use addEventListener.

You can simplify parsing the URL. document.location already is provides the parts of the URL. In your case document.location.pathname contains the path without domain, query, hash, etc:

function getFileName() {
    var url = document.location.pathname;
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if (url.length == 0) url = "index";
    return url;
}

In my opinion you are misusing the id attribute here. This is not what it's made for. (However to be honest I have difficulties finding the right words to explain why not - it just feels wrong.)

Finally you should be aware that this is a bad usage of JavaScript. You are using JS here for something that should be done server-side (that is directly in the HTML).


Serial port for Node.js I am trying to create a class for serial port using Node.js, but I want to know if there is a better way to write my data class code.

In my sample code below, in the getPortName prototype in the forEach loop, I declare a port object to store the serial port information before pushing into an array.

Is this a good way of declaring this port object? I think it is not good because it is difficult for users to see what the objects' members contain. Is there any way to make it clearly expose the port object member? Or is there a better way to improve this code structure?

'use strict';

var serialPort = require("serialport");

function Serial() {
    this.ComName = "";
}

Serial.prototype.getPortName = function() {
    serialPort.list(function (err, ports) {
        var availablePorts = [];

        ports.forEach(function(port) {
            var port = {
                 comName: "",
                 manufacturer: "",
                 pnpId:
            }

            port.comName = port.comName;
            port.manufacturer = port.manufacturer;
            port.pnpId = port.pnpId;
            availablePorts.push(port);
        });
    });

    return availablePorts;
};

Serial.prototype.setCOMName = function(name) {
    this.ComName = name;
}

module.exports = Serial;

First off, you should probably allow a default ComName to be passed into the constructor, and the default should probably be null instead of an empty string (unless you have good reason to do otherwise).

function Serial(name) {
    this.ComName = name !== undefined ? name : null;
}

Next, getPortName is strange. You set it on Serial.prototype, which indicates it will be used as an instance method. In fact, however, it doesn't use instance state at all! As your code is currently laid-out, you would need to perform the following to call getPortName:

new Serial().getPortName()

In classical OOP parlance, this is a static function. It doesn't need instance state, so declare it on Serial itself, not Serial.prototype.

Serial.getPortName = function() { ... }

With that change, you can simply call Serial.getPortName(), as you would a static method in other languages.

Furthermore, your naming conventions are a little odd. You use ComName for the property but COMName for the setter. Use comName for the property for consistency. If you need the setter, use setComName. However, the setter is also unnecessary. Why not just access serial.comName directly?

Finally, these changes beg the question... why is this object-oriented code at all? The only state you appear to be representing is a single string with no actual methods on it. Why not just export getPortName and call it a day? If your code is more complex, using JavaScript's OO capabilities might be worth it, but otherwise, you don't need it. A simple function will do.