Angular 2 service to upload a batch job and poll for results I have to write a web interface for one of my homebrew projects. Since I'm not familiar with HTML and CSS I've decided to take heavy duty framework such as Angular 2. This is my first experiment - I'm uploading a batch job to a remote servlet with POST request, get tracking ID and start polling this id with 100 ms interval. When remote batch completes its job, user of this service will be able to get result via Promise. Since I've written this without any systemic learning of any underlying technologies, I'm not sure if my approach is correct at all.

import { Injectable } from '@angular/core';
import {Http, Response} from '@angular/http';
import 'rxjs/add/operator/toPromise';
import 'rxjs/add/operator/map';

enum Status { SUCCESS, FAIL }

export class Upload {
  constructor(public readonly status: Status,
    public readonly message: string,
    public readonly timestamp: Date)
  {}
}

@Injectable()
export class EmeraldBackendStorageService {

  constructor(private http: Http) { }

  upload(fileToUpload: any): Promise<Upload> {
    let input = new FormData();
    input.append("file", fileToUpload);
    return this.http.post("/emerald/storage/submit-content", input)
      .map((response: Response) => JSON.parse(response.text()))
      .toPromise().then((serverAnswer: any) => {
        if (serverAnswer['success']) {
          return this.subscribeSubmitStatusTracker(serverAnswer['trackingId'] as number);
        } else {
          throw new Error('POST request failed');
        }
      }).catch((err) => {
        return new Upload(Status.FAIL, err, new Date())
      });
  }

  private subscribeSubmitStatusTracker(trackingId: number) : Promise<Upload>  {
    return this.http.get("/emerald/storage/submit-status/" + trackingId)
      .map((response: Response) => JSON.parse(response.text()))
      .toPromise().then((serverAnswer: any) => {
        if (serverAnswer['status'] == 'PENDING') {
          return new Promise<Upload>((resolve) => {
            setTimeout(() => resolve(this.subscribeSubmitStatusTracker(trackingId)), 5000);
          });
        } else if (serverAnswer['status'] == 'SUCCESS') {
          let ts = serverAnswer['timestamp'] as number;
          return new Upload(Status.SUCCESS, null, new Date(ts));
        } else {
          throw new Error('Server can\'t process uploaded file');
        }
      }).catch((err) => {
        return new Upload(Status.FAIL, err, new Date())
      })
  }
}

First of all, there are some stylish issues in the code:

let input = new FormData();
// `input` is never reassigned so you should use `const` instead of `let`:
const input = new FormData();

input.append("file", fileToUpload);
// you should use single-quotes for strings:
input.append('file', fileToUpload);

this.http.get("/emerald/storage/submit-status/" + trackingId)
// you can use string interpolation here:
this.http.get(`/emerald/storage/submit-status/${trackingId}`)

if (serverAnswer['status'] == 'PENDING') { }
// you should use triple-equals:
if (serverAnswer['status'] === 'PENDING') { }

and some semicolons are missing.

If you use @angular/cli with this project, I suggest ng lint to point and fix thoses little issues.

In the Upload class, we are expecting a message as a string but in case of failure we are passing an Error:

.catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));

Moreover about the Upload class, I think it can be misleading that timestamp member is typed as a Date, if it's a Date it should be named date or it should be typed as a number.

Then I think you can simplify the code with interfaces representing the server responses, here is a fixed and commented version of the code:

import { Injectable } from '@angular/core';
import { Http, Response } from '@angular/http';
import 'rxjs/add/operator/toPromise';
import 'rxjs/add/operator/map';

// it's possible to use string enum since typescript 2.4
// https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-4.html
export enum Status { PENDING = 'PENDING', SUCCESS = 'SUCCESS', FAIL = 'FAIL'}

export interface ContentResponse {
  readonly success: boolean;
  readonly trackingId: number;
}

export interface StatusResponse {
  readonly status: Status;
  readonly timestamp: number;
}

export class Upload {
  constructor(public readonly status: Status,
              public readonly message: string,
              public readonly date: Date) { // renamed timestamp -> date
  }
}

@Injectable()
export class EmeraldBackendStorageService {

  constructor(private http: Http) {
  }

  upload(fileToUpload: any): Promise<Upload> {
    const input = new FormData();
    input.append('file', fileToUpload);
    return this.http.post('/emerald/storage/submit-content', input)
       // we don't need to manually json parse the response,
       // response has already a method for that exact purpose
      .map((response: Response) => response.json())
      // we assume that we get a ContentResponse from the server
      // so it's easier to use our serverAnswer
      .toPromise().then((serverAnswer: ContentResponse) => {
        if (!serverAnswer.success) {
          throw new Error('POST request failed');
        }
        return this.subscribeSubmitStatusTracker(serverAnswer.trackingId);
      }).catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));
  }

  private subscribeSubmitStatusTracker(trackingId: number): Promise<Upload> {
    return this.http.get(`/emerald/storage/submit-status/${trackingId}`)
      .map((response: Response) => response.json())
      .toPromise().then((serverAnswer: StatusResponse) => {
        if (serverAnswer.status === Status.PENDING) {
          return new Promise<Upload>((resolve) => {
            setTimeout(() => resolve(this.subscribeSubmitStatusTracker(trackingId)), 5000);
          });
        } else if (serverAnswer.status === Status.SUCCESS) {
          return new Upload(Status.SUCCESS, null, new Date(serverAnswer.timestamp));
        } else {
          throw new Error(`Server can't process uploaded file`);
        }
      }).catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));
  }
}

Finally, an api returning a Promise<Upload> and the use of the toPromise operator is clearly not the angular way. In angular almost everything is an observable so the users of your api will expect observables.

As suggested by NewtonCode, you should use rxjs to achieve the functionality and return an Observable<Upload>.

Here is a rewrite with observables in mind:

import { Injectable, InjectionToken, Inject } from '@angular/core';
import { Http, Response } from '@angular/http';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/observable/of';
import 'rxjs/add/observable/interval';
import 'rxjs/add/observable/throw';
import 'rxjs/add/operator/catch';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/mergeMap';
import 'rxjs/add/operator/filter';
import 'rxjs/add/operator/take';

export enum Status { PENDING = 'PENDING', SUCCESS = 'SUCCESS', FAIL = 'FAIL'}

export interface ContentResponse {
  readonly success: boolean;
  readonly trackingId: number;
}

export interface StatusResponse {
  readonly status: Status;
  readonly timestamp: number;
}

// I need to configure the interval of time between request for testing purpose,
// because `fakeAsync`seems to have an issue with setInterval in Observable
// https://github.com/angular/angular/issues/10127
export interface EmeraldConfig {
  readonly intervalBetweenTrackingRequests: number;
}

export const EMERALD_CONFIG = new InjectionToken<EmeraldConfig>('EmeraldConfig');

export class Upload {
  constructor(public readonly status: Status,
              public readonly message: string,
              public readonly date: Date) {
  }
}

export const PostRequestFailedErrMsg = `POST request failed`;
export const UploadNotProcessedErrMsg = `Server can't process uploaded file`;

@Injectable()
export class EmeraldBackendStorageService {

  constructor(private http: Http, @Inject(EMERALD_CONFIG) private config: EmeraldConfig) {
  }

  upload(fileToUpload: any): Observable<Upload> {
    const input = new FormData();
    input.append('file', fileToUpload);

    return this.http.post('/emerald/storage/submit-content', input)
      .map((response: Response) => response.json())
      // we emit an error if the upload is a failure
      .map((serverAnswer: ContentResponse): ContentResponse => {
        if (!serverAnswer.success) {
          throw new Error(PostRequestFailedErrMsg);
        }
        return serverAnswer;
      })
      // then we return an observable emitting the succesful tracking response
      .mergeMap((serverAnswer: ContentResponse): Observable<StatusResponse> => this.track(serverAnswer.trackingId))
      .map((serverAnswer: StatusResponse): Upload => new Upload(Status.SUCCESS, null, new Date(serverAnswer.timestamp)))
      // finally we recover, but only if it's
      // a upload not processed error or a post request failed error
      .catch((err: Error) => (err.message === UploadNotProcessedErrMsg || err.message === PostRequestFailedErrMsg) ?
        Observable.of(new Upload(Status.FAIL, err.message, new Date())) : Observable.throw(err));
  }

  private track(trackingId: number): Observable<StatusResponse> {
    // we create an observable that emits every X ms
    return Observable.interval(this.config.intervalBetweenTrackingRequests)
      // then with mergeMap we are calling the tracking service every X ms and emitting the responses with an observable
      .mergeMap(() => this.http.get(`/emerald/storage/submit-status/${trackingId}`))
      .map((response: Response): StatusResponse => response.json())
      // we discard the pending responses
      .filter((serverAnswer: StatusResponse) => serverAnswer.status !== Status.PENDING)
      // and complete our observable if we get a success or fail response
      .take(1)
      // finally, we emit an error if the response is fail
      .map((serverAnswer: StatusResponse): StatusResponse => {
        if (serverAnswer.status !== Status.SUCCESS) {
          throw new Error(UploadNotProcessedErrMsg);
        }
        return serverAnswer;
      });
  }
}

As you can see, it's quite different from the promised code, but you will see with practice that observables are more powerful than promises.

You can easily limit the number of calls to the tracking service:

    return Observable.interval(this.config.intervalBetweenTrackingRequests)
      .take(3) // only 3 attempts
      .mergeMap(() => this.http.get(`/emerald/storage/submit-status/${trackingId}`))
      .map((response: Response): StatusResponse => response.json())
      .filter((serverAnswer: StatusResponse) => serverAnswer.status !== Status.PENDING)
      .take(1)

Or add a timeout to the entire process:

  upload(fileToUpload: any): Observable<Upload> {
    const input = new FormData();
    input.append('file', fileToUpload);

    return this.http.post('/emerald/storage/submit-content', input)
      .map((response: Response) => response.json())
      ... // all the operations
      .timeout(20000);
  }

Bonus

the spec of the observable version:

import { async, TestBed } from '@angular/core/testing';
import { BaseRequestOptions, Response, Http, ResponseOptions } from '@angular/http';
import { MockBackend } from '@angular/http/testing';
import {
  EmeraldBackendStorageService, Status, StatusResponse,
  ContentResponse, Upload, EMERALD_CONFIG, UploadNotProcessedErrMsg, PostRequestFailedErrMsg
} from './emerald-backend-storage.service';

describe('EmeraldBackendStorageService', () => {
  let emerald: EmeraldBackendStorageService;

  const id3AndSuccessContentResponse: ContentResponse = {success: true, trackingId: 3};
  const id0AndFailContentResponse: ContentResponse = {success: false, trackingId: 0};

  const timestamp400AndSuccessTrackingResponse: StatusResponse = {status: Status.SUCCESS, timestamp: 400};
  const timestamp400AndPendingTrackingResponse: StatusResponse = {status: Status.PENDING, timestamp: 400};
  const timestamp5400AndSuccessTrackingResponse: StatusResponse = {status: Status.SUCCESS, timestamp: 5400};
  const timestamp5400AndFailTrackingResponse: StatusResponse = {status: Status.FAIL, timestamp: 5400};

  function respond(contentResponse: ContentResponse, ...statusResponses: Array<StatusResponse>): void {
    const mockBackend = TestBed.get(MockBackend);

    mockBackend.connections.subscribe(connection => {
      const response = connection.request.url === '/emerald/storage/submit-content' ?
        contentResponse : statusResponses.shift();

      connection.mockRespond(new Response(new ResponseOptions({body: response})));
    });
  }

  beforeEach(() => {
    TestBed.configureTestingModule({
      providers: [
        EmeraldBackendStorageService,
        {
          provide: Http,
          useFactory: (backend, defaultOptions) => new Http(backend, defaultOptions),
          deps: [MockBackend, BaseRequestOptions]
        },
        {
          provide: EMERALD_CONFIG,
          useValue: {delay: 5}
        },
        MockBackend,
        BaseRequestOptions
      ]
    });
    emerald = TestBed.get(EmeraldBackendStorageService);
  });

  it(`should return an observable with an Upload{'SUCCESS', null, 400},
                 when successfully uploading a file
                 and the tracking response is {'SUCCESS', 400}`,
    async(() => {
      respond(id3AndSuccessContentResponse, timestamp400AndSuccessTrackingResponse);

      emerald.upload('myFile').subscribe((upload: Upload) => {
        expect(upload.status).toBe(Status.SUCCESS);
        expect(upload.date.getTime()).toBe(400);
        expect(upload.message).toBeNull();
      });
    }));

  it(`should return an observable with an Upload{'SUCCESS', null, 5400},
                 when successfully uploading a file
                 and the tracking responses are {'PENDING', 400} then {'SUCCESS', 5400}`,
    async(() => {
      respond(id3AndSuccessContentResponse,
        timestamp400AndPendingTrackingResponse, timestamp5400AndSuccessTrackingResponse);

      emerald.upload('myFile').subscribe((upload: Upload) => {
        expect(upload.status).toBe(Status.SUCCESS);
        expect(upload.date.getTime()).toBe(5400);
        expect(upload.message).toBeNull();
      });
    }));

  it(`should return an observable with an Upload{'FAIL', 'POST request failed', ...},
                 when upload response is {false, 0}`,
    async(() => {
      respond(id0AndFailContentResponse);

      emerald.upload('myFile').subscribe((upload: Upload) => {
        expect(upload.status).toBe(Status.FAIL);
        expect(upload.message).toBe(PostRequestFailedErrMsg);
      });
    }));

  it(`should return an observable with an Upload{'FAIL', 'Server can't process uploaded file', ...},
                 when successfully uploading a file
                 and the tracking responses are {'PENDING', 400} then {'FAIL', 5400}`,
    async(() => {
      respond(id3AndSuccessContentResponse,
        timestamp400AndPendingTrackingResponse, timestamp5400AndFailTrackingResponse);

      emerald.upload('myFile').subscribe((upload: Upload) => {
        expect(upload.status).toBe(Status.FAIL);
        expect(upload.message).toBe(UploadNotProcessedErrMsg);
      });
    }));
});


Custom MySQLi connector class for PHP I have written a class that connects to a database using MySQLi and has methods for the connection itself and returns the instance of the MySQLi class.

This works, but feels a little "clunky" and can be a bit slow, can someone help me to improve this?

The class is as follows:

<?php

/**
 * Connect to a db.
 */
class mysqli_connector
{
    /** Private to implement Singleton pattern */
    private function __construct() {  }

    private static $init;

    /** @return mysqli_connector */
    public static function Init()
    {
        return static::$init = (
                null === static::$init ? new self() : static::$init
            );
    }
    /** @var mysqli */
    private static $link;
    /**
     * Sets and gets the link to the database based on params
     * @param string $host
     * @param string $username
     * @param string $password
     * @param string $dbname
     * @return mysqli
     */
    public function link($host = '', $username = '', $password = '', $dbname = '')
    {
        static::$link = mysqli_init();
        if ($host != '' && $username != '' && $password != '' && $dbname != '')
        {
            static::$link->real_connect($host, $username, $password, $dbname);
        }
        else if ($host != '' && $username != '' && $password != '' && $dbname == '')
        {
            static::$link->real_connect($host, $username, $password);
        }
        else if ($host != '' && $username != '' && $password == '' && $dbname == '')
        {
            static::$link->real_connect($host, $username);
        }
        else if ($host != '' && $username == '' && $password == '' && $dbname == '')
        {
            static::$link->real_connect($host);
        }
        else
        {
            static::$link->real_connect();
        }
        return static::$link;
    }
}

EDIT

I use the class as such:

<?php

require_once 'include/class/_db/mysqli-connector.php';

$connector = mysqli_connector::Init();
$link = $connector->link("myhost", "mysuser", "mypass", "mydbname");
var_dump($link); # Make sure it works!

?>

Output from the var_dump($link);

object(mysqli)[2]
    public 'affected_rows' => int 0
    public 'client_info' => string 'mysqlnd 5.0.11-dev - 20120503 - $Id: /* removed for security */ $' (length=79)
    public 'client_version' => int 50011
    public 'connect_errno' => int 0
    public 'connect_error' => null
    public 'errno' => int 0
    public 'error' => string '' (length=0)
    public 'error_list' =>
        array (size=0)
            empty
    public 'field_count' => int 0
    public 'host_info' => string 'localhost via TCP/IP' (length=20)
    public 'info' => null
    public 'insert_id' => int 0
    public 'server_info' => string '5.6.26' (length=6)
    public 'server_version' => int 50626
    public 'stat' => string 'Uptime: 118450  Threads: 1  Questions: 149  Slow queries: 0  Opens: 70  Flush tables: 1  Open tables: 63  Queries per second avg: 0.001' (length=135)
    public 'sqlstate' => string '00000' (length=5)
    public 'protocol_version' => int 10
    public 'thread_id' => int 117
    public 'warning_count' => int 0

First, don't use else. Or try to avoid it. The link method can be reduced drastically by just reading the docs.

host
Can be either a host name or an IP address. Passing the NULL value or the string "localhost" to this parameter, the local host is assumed. When possible, pipes will be used instead of the TCP/IP protocol.

Usually, using NULL in a variable will help you achieve the default behavior of things. Take advantage of it.
/**
 * Sets and gets the link to the database based on params
 * @param string $host
 * @param string $username
 * @param string $password
 * @param string $dbname
 * @return mysqli
 */
public function link($host = null, $username = null, $password = null, $dbname = null)
{
    static::$link->real_connect($host, $username, $password, $dbname);
    return static::$link;
}

Secondly, why is $link static? You're already using the Singleton Pattern, to make the $link variable static is double redundant against the pattern.


Simple binary heap in C# I've written a simple binary heap in C# and I want to know if it has any problems or if I can make it better.

public enum HeapType
{
    MinHeap,
    MaxHeap
}

public class Heap<T> where T : IComparable<T>
{
    List<T> items;

    public HeapType MinOrMax { get; private set; }

    public T Root
    {
        get { return items[0]; }
    }

    public Heap(HeapType type)
    {
        items = new List<T>();
        this.MinOrMax = type;
    }

    public void Insert(T item)
    {
        items.Add(item);

        int i = items.Count - 1;

        bool flag = true;
        if (MinOrMax == HeapType.MaxHeap)
            flag = false;

        while(i > 0)
        {
            if ((items[i].CompareTo(items[(i - 1) / 2]) > 0) ^ flag)
            {
                T temp = items[i];
                items[i] = items[(i - 1) / 2];
                items[(i - 1) / 2] = temp;
                i = (i - 1) / 2;
            }
            else
                break;
        }
    }

    public void DeleteRoot()
    {
        int i = items.Count - 1;

        items[0] = items[i];
        items.RemoveAt(i);

        i = 0;

        bool flag = true;
        if (MinOrMax == HeapType.MaxHeap)
            flag = false;

        while(true)
        {
            int leftInd = 2 * i + 1;
            int rightInd = 2 * i + 2;
            int largest = i;

            if (leftInd < items.Count)
            {
                if ((items[leftInd].CompareTo(items[largest]) > 0) ^ flag)
                    largest = leftInd;
            }

            if (rightInd < items.Count)
            {
                if ((items[rightInd].CompareTo(items[largest]) > 0) ^ flag)
                    largest = rightInd;
            }

            if (largest != i)
            {
                T temp = items[largest];
                items[largest] = items[i];
                items[i] = temp;
                i = largest;
            }
            else
                break;
        }
    }

    public T PopRoot()
    {
        T result = items[0];

        DeleteRoot();

        return result;
    }
}

I note that you're restricting your types T to those that are IComparable, that is, those that implement CompareTo. A more general solution would allow the caller to specify their own IComparer which may be that type's CompareTo or may be something else entirely.

Also see https://stackoverflow.com/questions/14336416/using-icomparer-for-sorting


Print permutations in an iterative way I've written the following code to print all the permutations of a list. For example:

  \$[1, 2, 3]\$ has these permutations: \$[1,2,3]\$, \$[1,3,2]\$, \$[2,1,3]\$, \$[2,3,1]\$, \$[3,1,2]\$, and \$[3,2,1]\$.

def insert_each_position(self, nums, insert_one):
    res, len_of_nums = [], len(nums)
    for idx in range(len_of_nums+1):
        tmp = nums[:]
        tmp.insert(idx, insert_one)
        res.append(tmp)
    return res

def permute(self, nums):
    res, len_of_nums = [[]], len(nums)
    for idx in range(len_of_nums):
        cur_num = nums[idx]
        tmp = []
        for sub in res:
            tmp.extend(self.insert_each_position(sub, cur_num))
        res = tmp
    return res

As suggested by @netme in the comments, unless you need to write the permutation code yourself, it would be much better to use itertools.permutations, as so:

import itertools

for perm in itertools.permutations([1, 2, 3]):
    print(perm)

That code is more compact, memory-efficient and faster than yours. The itertools module has been deliberately honed to be fast (I think it’s written in C rather than pure Python) and is worth looking into if you haven’t used it before – it’s really useful.

Assuming that you're supposed to be writing your own permutation, here are some comments on that code.

(Given the self parameter and the self.insert_each_position in the second function, I’m assuming these are class methods. I put them in their own class for testing.)

Unless tuple unpacking makes the code cleaner or the variables are related, I'm generally inclined to split variable definitions over multiple lines. So I’d split the variable declarations over two lines in the start of each function.

I would then get rid of the len_of_nums variable. It’s much clearer just to use len(nums) directly in the two places where this variable is used.
You don’t mention whether you’re using Python 2 or 3. If you’re using 2, you should use xrange() instead of range(), which is slightly faster and more memory efficient.
There are no comments or docstrings. I had to guess how the function worked. You should explain what the code is trying to achieve, and why you wrote it that way – it will make it easer for me to review, and other people to use.

I would also suggest trying to pick better variable names. res and tmp are not particularly descriptive or useful.
My concern with your approach is that you gradually build up a list of all the permutations, which is going to be very memory inefficient and slow. Your program began to struggle when I asked it to permute range(10). It think it has exponential growth, but I’m not sure.

If you want something fast and you can’t use itertools, I suggest you consider looking at generators for yielding individual permutations. That’s a lot more memory efficient, and will be much faster.


POSIX shell script for generating pseudo-random numbers My today's goal was to further practice POSIX shell scripting.

So, I decided to write the following POSIX shell script for generating pseudo-random numbers.

Any and all reviews would be appreciated.

#!/bin/sh

bold=$(tput bold)
red=$(tput setaf 1)
nocolor=$(tput sgr0)

bold_red="${bold}${red}"

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    if [ "$#" -ne 2 ]
    then
        print_error_and_exit "print_error_and_exit(): There have not been passed two arguments!" 2
    fi

    # check if the second argument is a number
    if ! is_this_a_number "$2"
    then
        print_error_and_exit "print_error_and_exit(): The argument #2 is not a number!" 5
    fi

    echo "${bold_red}$1 Exit code = $2.${nocolor}" 1>&2
    exit "$2"
}

is_this_a_number()
{
    # check if exactly one argument has been passed
    if [ "$#" -ne 1 ]
    then
        print_error_and_exit "is_this_a_number(): There has not been passed exactly one argument!" 2
    fi

    # check if the argument is an integer number
    if [ "$1" -eq "$1" ] 2> /dev/null
    then
        return 0
    else
        return 1
    fi
}

random_number()
{
    # check if exactly two arguments have been passed
    if [ "$#" -ne 2 ]
    then
        print_error_and_exit "random_number(): There have not been passed exactly two arguments!" 3
    fi

    # check if the first argument is a number
    if ! is_this_a_number "$1"
    then
        print_error_and_exit "random_number(): The argument #1 is not a number!" 4
    fi

    # check if the second argument is a number
    if ! is_this_a_number "$2"
    then
        print_error_and_exit "random_number(): The argument #2 is not a number!" 5
    fi

    # min is inclusive
    range_min="$1"

    # max is inclusive too
    range_max="$2"

    # RANDOM is not POSIX-available,
    # so we can use shuf for instance
    shuf -i "$range_min"-"$range_max" -n 1
}

# temporary testing loop
i=1
while [ "$i" -lt 10 ]
do
    random_number 1 9

    i=$((i + 1))
done

Firstly, it's good that you use tput to portably obtain terminal codes if they exist.  Too many shell script authors fail to do that!  We might choose to only evaluate those if we know we'll use them (i.e. inside print_error_and_exit), and there seems little point in setting $bold_red to use only once.
I wonder if the error checking within print_error_and_exit might be overkill.  If you're using it only within this script, you might omit the argument checking (if you made a mistake in the recursive call, you might end up in a worse position than not checking at all!).
It might be worth accepting the arguments to print_error_and_exit in the opposite order (exit code first), to avoid the important part being overlooked at the end of the line.
Some of the if tests are very verbose compared to simply chaining commands with && or ||.  That might be a style decision, but I would certainly reconsider
if command
then
    return 0
else
    return 1
fi

When it's the last line of the function, that simplifies to just
command

(unless you really care which non-zero value indicates failure).
Although we know that $# expands to an integer, and is therefore safe to use without quoting, there's nothing wrong with quoting it as you have (and here on Stack Exchange it helps the prettifier distinguish it from a comment).
I was concerned that shuf might be very inefficient if a large range is selected, but reading the source for the GNU coreutils implementation shows that it manages to avoid building the entire list of numbers to choose from.  Whether other implementations are similarly optimised is left as an exercise.
Modified program
#!/bin/sh

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    test "$#" = 2 || print_error_and_exit 2 "print_error_and_exit(): There have not been passed two arguments!"

    # check that the first argument is a number
    is_this_a_number "$1" || print_error_and_exit 5 "print_error_and_exit(): The argument #1 is not a number!"

    bold=$(tput bold)
    red=$(tput setaf 1)
    nocolor=$(tput sgr0)

    echo "$bold$red$2 Exit code = $1.$nocolor" >&2
    exit $1
}

is_this_a_number()
{
    # check if exactly one argument has been passed
    test "$#" = 1 || print_error_and_exit 2 "is_this_a_number(): There has not been passed exactly one argument!"
    # check if the argument is an integer
    test "$1" -eq "$1" 2>/dev/null
}

# print a number in range $1 to $2, inclusive
random_number()
{
    # check if exactly two arguments have been passed
    test "$#" = 2 || print_error_and_exit 3 "random_number(): There have not been passed exactly two arguments!"

    # check if the arguments are both numbers
    is_this_a_number "$1" || print_error_and_exit 4 "random_number(): The argument #1 is not a number!"
    is_this_a_number "$2" || print_error_and_exit 5 "random_number(): The argument #2 is not a number!"

    shuf -i "$1-$2" -n 1
}


Fizzbuzz questions - job interview rejected I have done some programming tasks as a part of my candidacy for a web development apprenticeship. Unfortunately, my application was rejected as the hiring manager "didn't like my codes." I called up their office but no one was able to provide sufficient feedback for me.
If any of you could tell me what I could have misunderstood or what I could have done correctly, I pretty much appreciate it.
The Tasks I worked with
Write a program that shows the numbers from 1 to a specified limit (e.g. 100).
The numbers should be translated as follows: 1. For multiples of 3 return "Fizz" 2. For multiples of 5 return "Buzz" 3. For multiples of 3 and 5 return "FizzBuzz" 4. If none of the other rules apply, return the number itself
Unit tests are optional, but would be a plus.
My Solution
public class FizzBuzz {

    //a class *variable*, not a class
    private static int counter;

    //checking if a multiple of 3
    private static boolean isFizz(int i) {
        return i % 3 == 0; //no need for if
    }

    //checking if a multiple of 5
    private static boolean isBuzz(int i) {
        return i % 5 == 0; //no need for if
    }

    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            System.out.println(i);
            if (i % 5 == 0 && i % 3 == 0) {
                System.out.println("FizzBuzz");
            } else if (i % 3 == 0){
                System.out.println("Fizz");
            } else if (i % 5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(i);
            }
        }
    }

    @Test public void testFizzBuzz(){
        assertEquals("fizz", fizzBuzz(i == 3));
        assertEquals("buzz", fizzBuzz(i == 5));
        assertEquals("fizzbuzz", fizzBuzz(i == 15));
        assertEquals("2", fizzBuzz(i == 2));
    }

}

You don't use counter, isFizz() or isBuzz(), testFizzBuzz() calls to a fizzBuzz method which doesn't appear to exist. (Passing a boolean argument which seems illogical).
Your code is formatted well, but the points above mean that a reviewer would doubt your competence.
Have you shown us all your code? If not, why not?
Did you compile, run and test your code? If not, why not?


Excel to CSV converter To check out the cssv module i wrote a small programm which takes all xlsx files in a folder and converts them into csv files.

Let me know what you would improve on this program.

xlsx_tp_csv

"""
All xlsx files in the folder of the script get converted into csv files
(One for each sheet).
The names are created as excel filename + sheet name.
"""

import csv
import os
from pathlib import Path
import openpyxl

def make_csv_filename(filename_excel: str, sheet_name: str) -> str:
    """
    Make a filename out of the filename of the excel file and
    the corresponding sheet
    """
    return Path(filename_excel).resolve().stem  + '_' + sheet_name + '.csv'

def xlsx_to_csv():
    """Main loop"""
    target_folder: str = 'csv'
    os.makedirs(target_folder, exist_ok=True)

    for filename in os.listdir('.'):
        if not filename.endswith('.xlsx'):
            continue

        workbook = openpyxl.load_workbook(filename)

        for sheet_name in workbook.sheetnames:
            sheet = workbook[sheet_name]
            csv_filename = make_csv_filename(filename, sheet_name)
            csv_path: Path = Path(target_folder, csv_filename)

            with open(csv_path, 'w', newline='') as csv_file:
                csv_writer = csv.writer(csv_file)

                for row_number in range(1, sheet.max_row + 1):
                    row_data = []
                    for column_number in range(1, sheet.max_column + 1):
                        cell_data = sheet.cell(
                            column=column_number, row=row_number).value
                        row_data.append(cell_data)

                    csv_writer.writerow(row_data)

if __name__ == "__main__":
    xlsx_to_csv()

Good job with the type annotations! Here are a few things you could simplify/improve:

instead of listdir and an extra file extension check, you could use glob.iglob():

for filename in glob.iglob('./**/*.xlsx'):

to improve excel file read performance and memory consumption, you could use read_only=True mode
instead of iterating over sheetnames and getting a sheet by name, you could iterate over available worksheets directly:

for sheet in workbook.worksheets:

iter_rows() should improve the way you read the sheet cells

As a side note and as an idea for an alternative solution: what if you transition the data through a pandas.DataFrame, pandas has .read_excel() and .to_csv() methods.


Selecting HTML nodes I'm new to Clojure, and I'm writing some code that processes HTML. I'm parsing the HTML using clj-tagsoup and then trying to pull out the relevant bits with this function:

(defn find-nodes [tag-prototype base-node]
  (letfn [(node-matches [tag-prototype node]
            (let [[tag attrs] tag-prototype]
              (and (= (nth node 0) tag)
                   (every? (fn [[key val]]
                             (= (key (nth node 1)) val))
                           (seq attrs)))))]
    (if (node-matches tag-prototype base-node)
      [base-node]
      (loop [nodes base-node matches []]
        (if (seq nodes)
          (recur (next nodes)
                 (let [node (first nodes)]
                   (if (vector? node)
                     (if (node-matches tag-prototype node)
                       (conj matches node)
                       (concat matches (find-nodes tag-prototype node)))
                     matches)))
          matches)))))

Used something like this:

(->> (tagsoup/parse file)
     (find-nodes [:div {:class "magic"}])
     (find-nodes [:p]))

The code works (finally), but the style is probably horrible. What would you suggest to improve it?

Sample input:

<!DOCTYPE html>

    <title>This is a test page</title>

        ...

        Hello World.

Sample query:

(pprint (find-nodes [:div {:id "something-important"}]
                    (tagsoup/parse "test.html")))

Sample output:

([:div
  {:id "something-important"}
  [:p {:class "alert"} "\n        ...\n      "]])

Sample query:

(pprint (find-nodes [:p]
                    (tagsoup/parse "test.html")))

Sample output:

([:p {:class "alert"} "\n        ...\n      "]
 [:p {:class "message"} "\n        Hello World.\n      "])

First things first: any time you find yourself using letfn, you should strongly consider simply breaking those functions out into their own defns. In this case, node-matches is large enough and decoupled enough to make that clearly the right choice.

One way you can make your code more concise is to take advantage of the fact that you can do destructuring directly in function arguments:

(defn node-matches [[tag attrs] [node-tag node-attrs]]
  (and (= node-tag tag)
       (every? (fn [[key val]]
                 (= (key node-attrs) val))
               (seq attrs))))

If you want to make clear what it is that you're destructuring, you could add :as prototype and :as node to your parameters.

When you think about it, though, that every? expression is really just trying to determine if attrs is a subset of node-attrs. In Clojure, you can use the subset? function to do that. So, assuming that you've already required [clojure.set :as set]:

(set/subset? (set attrs) (set node-attrs))

A lot of the complexity in the rest of find-nodes comes from the fact that you're complecting the logic that traverses the HTML tree and the logic that uses node-matches to do the matching and assemble the final result.

The best way to improve find-nodes, then, is to break it apart. Actually, traversing a Hiccup-style HTML tree is incredibly easy in Clojure using the tree-seq function:

(tree-seq vector? tagsoup/children (tagsoup/parse "test.html"))

This will give you a sequence of all the nodes in the HTML tree:

([:html {}
  [:head {}
   [:title {} "This is a test page"]]
  [:body {}
   [:div {:id "something-important"}
    [:p {:class "alert"}
     "\n        ...\n      "]]
   [:div {:id "something-else"}
    [:p {:class "message"}
     "\n        Hello World.\n      "]]]]
 [:head {}
  [:title {} "This is a test page"]]
 [:title {} "This is a test page"]
 "This is a test page"
 [:body {}
  [:div {:id "something-important"}
   [:p {:class "alert"}
    "\n        ...\n      "]]
  [:div {:id "something-else"}
   [:p {:class "message"}
    "\n        Hello World.\n      "]]]
 [:div {:id "something-important"}
  [:p {:class "alert"}
   "\n        ...\n      "]]
 [:p {:class "alert"}
  "\n        ...\n      "]
 "\n        ...\n      "
 [:div {:id "something-else"}
  [:p {:class "message"}
   "\n        Hello World.\n      "]]
 [:p {:class "message"}
  "\n        Hello World.\n      "]
 "\n        Hello World.\n      ")

And at this point, you're pretty much done. You just need to filter this sequence using the shortened version of node-matches that we wrote above:

(defn find-nodes [[tag attrs] root]
  (filter (fn [[node-tag node-attrs]]
            (and (= tag node-tag) (set/subset? (set attrs) (set node-attrs))))
          (tree-seq vector? tagsoup/children root)))


Simple input class change with vanilla JS I am just testing how I solved simple stuff.

Goal: The aim is to expand the width of an input and collapse it.

When mouseover expand,.

When mouseout collapse.

When you have text inside the input keep it expanded.

To me it seems too much, repeating some stuff in JS.
Also thought about adding a debouncer or throttle but was not successful.

There's probably another way to solve it with pure CSS.

HTML:

    2 inputs, 1 is collapsed
    the other is expanded

collapsed

    <input type="text" class="collapsed">

    expanded

    <input type="text" class="expanded">

CSS:

input {
    width: 20px;
    transition: width 1s;
}

.expanded{
    width: 256px;
}

JS:

let collapsedInput = document.querySelector('.collapsed');

const handleMouseOut = () => {
    collapsedInput.classList.remove('expanded');
}

const handleMouseOVer = () => {
    collapsedInput.classList.add('expanded');
}

collapsedInput.addEventListener('mouseover', handleMouseOVer);

collapsedInput.addEventListener('mouseout', handleMouseOut);

collapsedInput.addEventListener('input', () => {
    if(collapsedInput.value !== ""){
        collapsedInput.classList.add('expanded');
        collapsedInput.removeEventListener('mouseout', handleMouseOut);
    } else {
        collapsedInput.classList.remove('expanded');
        collapsedInput.addEventListener('mouseout', handleMouseOut);
    }
})

Review

The biggest critique about the Javascript I have is that collapsedInput could be declared with const because it doesn't get re-assigned.

It is possible that the code in the event listener for the input event could be simplified using classList.toggle().

If there is only one element with class name collapsed then perhaps it would be better to use an id attribute and use document.getElementById() to select it. Those older methods are faster than the querySelector() varieties.

Dramatic simplification

The question contained this text:

  There's probably another way to solve it with pure CSS.

That is correct. One way is to use the required attribute on the first <input> element along with the :valid and :hover pseudo-selectors :

input {
    width: 20px;
    transition: width 1s;
}

input:valid,
input:hover,
.expanded {
    width: 256px;
}

    2 inputs, 1 is collapsed
    the other is expanded

collapsed

    <input type="text" class="collapsed" required>

    expanded

    <input type="text" class="expanded">

Perhaps someday the CSS pseudo-class :blank will be supported by some browsers... If that is the case, then the required attribute wouldn't be needed.


Bash script to perform LUHN check The following Bash script takes a single parameter (the PAN) and exits with 1 if the PAN does not satisfy a  LUHN check.

#!/bin/bash

pan=$1
panlen=${#pan}

for i in $(seq $((panlen - 1)) -1 0); do
  digit=${pan:$i:1}
  if [ $(((panlen-i) % 2)) -eq 0 ]; then
     #even
     ((digit*=2))
     [ ${#digit} -eq 2 ] && digit=$((${digit:0:1}+${digit:1:1}))
  fi
  ((sum+=digit))
done

[ $((sum % 10)) -eq 0 ] || exit 1

For example, when placed in a file called isLUHNValid.sh it can be run with a valid PAN like:

./isLUHNValid.sh 4388576018410707 && echo 'Valid PAN' || echo 'Invalid PAN'

which will echo 'Valid PAN'.

And with an invalid PAN:

./isLUHNValid.sh 4388576018410708 && echo 'Valid PAN' || echo 'Invalid PAN'

it will echo 'Invalid PAN'.

I'm interested to know:

if I've followed best practices; and/or
if there's an opportunity for optimisation

I've been using Bash for a couple of years - but I'm constantly learning new things from examples on the Internet (like from here) and from man bash. Unfortunately in my position I don't have anyone to look over my shoulder and review by Bash code so I thought I'd take advantage of this community.

Unfortunately in my position I don't have anyone to look over my shoulder and review by Bash code so I thought I'd take advantage of this community.

My first reaction (as usual with any Bash review) was to copy-paste your code into shellcheck.net, and say that you do have someone to check your back, but your code passes with flying colors, so congrats! And yeah, we all have this issue, I hope that Code Review can make a difference for you.

It's a nice script and it runs well, but it can be better.

seq is not recommended, because it's not portable. It's not needed anymore anyway, because any modern Bash natively supports traditional for (( ; ; )) loops as in other languages, without having to spawn a sub-process. So the main loop can be written as:

for ((i = panlen - 1; i >= 0; i--)); do

Notice there I wrote spaces around operators. It's not an official recommendation, but I like this writing style, as recommended in other languages for increased readability. You can apply this style for all the code within ((...)), so for example instead of this:

 ((digit*=2))

I would write:

 ((digit *= 2))

Since you are already using ((...)) and as such the script is not POSIX compliant, I recommend all the way, and use it more aggressively for a simplified and more natural writing style.

For example instead of this:

  if [ $(((panlen-i) % 2)) -eq 0 ]; then

You can write like this:

  if (((panlen - i) % 2 == 0)); then

Comparisons with numbers are more natural in an arithmetic context within ((...)) than the -eq operator within [ ... ].

The final line of the program can be changed from this:

[ $((sum % 10)) -eq 0 ] || exit 1

To this:

((sum % 10 == 0))

That is, using a natural arithmetic context,
and taking advantage that the exit code of the program will be the exit code of the last command, making the || exit 1 redundant.
You may still want to leave the || exit 1 for clarity, it's up to you.


Simulation of a falling ball I made a simple simulation of a falling ball. Is it possible to somehow improve or optimize this code? What tips can you give for development?
#include <iostream>
#include <cstdlib>
#include <cmath>
using namespace std;

int timeFallingBall() {
    static int time(0);
    return ++time;
}
void fallingBall() {
    cout << "From what height do we drop the ball? (In meters): ";
    double h;
    cin >> h;
    double v = 0, high = 0, s = 0, maxv = 0, g = 9.8;
    int t = 0;
    double hi = h;
    while (h) {
        t = timeFallingBall();
        v = static_cast<double>(t) * g;
        high = (v * static_cast<double>(t)) / 2;
        h = hi - high;
        if (h <= 0) {
            switch (t) {
            case 1:
                cout << "After " << t << " second the ball has reached the ground! Maximum speed: " << maxv << " m/s";
                break;
            default:
                cout << "After " << t << " seconds the ball has reached the ground! Maximum speed: " << maxv << " m/s";
            }
            break;
        }
        switch (t) {
        case 1:
            cout << "After " << t << " second, the ball is at a distance of " << h << " m from the ground at a speed: " << v << " m/s" << endl;
            break;
        default:
            cout << "After " << t << " seconds, the ball is at a distance of " << h << " m from the ground at a speed: " << v << " m/s" << endl;
            break;
        }
        if (v > maxv) {
            maxv = v;
        }
    }
}
int main() {
    system("chcp 1251>nul");
    fallingBall();
    return 0;
}

using namespace std; is a bad habit to get into, as it can lead to name-collisions and other issues. It's best to explicitly qualify the names we need where necessary, e.g. std::cout.

A few more line-breaks would be useful in the code. Think of code with a specific purpose as a paragraph when writing text. For example, this could be considered one "paragraph", so should have an empty line after it.
 std::cout << "From what height do we drop the ball? (In meters): ";
 double h;
 std::cin >> h;

We have several similarly named variables h, high and hi. This makes the code harder to understand.

Variables that don't change should be marked const. (e.g. g).

Variables should generally be declared as close to the point of use as possible, set to immediately useful values, and not reused (unless they consume significant resources). For example, t, v, high and h can all be declared where they are set inside the loop.

s does not appear to be used.

We have no way to reset the static time variable in timeFallingBall(). This means we can't call fallingBall a second time, to drop another ball. We can easily just increment t in the fallingBall function instead.

while (h) translates to while (h != 0). Since h is a double with a calculated value, it's a bad idea to compare it with an exact number. We should use while (h > 0.0) instead, or while (true) since we handle the exit condition inside the loop.

In C++, number literals have specific types: 0 is an int, 0.f is a float, 0.0 is a double. When comparing or assigning to variables, it's best to use the literal style of the correct variable type, rather than relying on conversions.

std::endl outputs a newline, but also flushes the output stream unnecessarily. We can output "\n" to get a newline without the flush.

Note that given the constant downwards acceleration, the maximum velocity is always going to be the velocity when the ball hits the ground. So we don't really need to keep track of it.

(Obviously the 1 second resolution makes the max speed output rather incorrect).

Applying the above, I'd probably go with something like:
void fallingBall()
{
    std::cout << "From what height do we drop the ball? (In meters): ";
    double h_start;
    std::cin >> h_start;

    if (h_start <= 0.0)
    {
        std::cout << "The ball is already on the ground!\n";
        return;
    }

    const double g = 9.81;

    double t = 0.0;

    while (true)
    {
        t += 1.0;

        const double h_current = h_start - 0.5 * g * t * t;
        const double v_current = g * t;

        if (h_current <= 0.0)
        {
            std::cout << "After second " << t << ", the ball has reached the ground! Maximum speed: " << v_current << " m/s\n";
            break;
        }

        std::cout << "After second " << t << ", the ball is at a distance of " << h_current << " m from the ground at a speed: " << v_current << " m/s\n";
    }
}


Measuring the clock rate of an Intel CPU in C++ I created a short program to directly measure the clock rate of an Intel CPU. It runs, sleeps for a duration specified by the user, and then measures the number of clock cycles and the amount of time that elapsed while it was asleep, then divides the elapsed clock cycles by the elapsed time to get the frequency of the CPU.

Is the below code up to stuff in terms of style and readability?
Is there any reason the strategy used and described below won't produce accurate results?

Any feedback is appreciated! If you wish to test out the code, you can download it with git clone https://github.com/firetotherain/CPUHertz.git

#include <thread>
#include <chrono>
#include <cstdio>

//Uses assembly command to get the current value of the cycle "counter"
uint64_t get_cycles()
{
    unsigned int lo,hi;
    __asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
    return ((uint64_t)hi << 32) | lo;
}

//Defines unit of time to measure seconds
typedef std::chrono::duration<double, std::ratio<1,1>> seconds_t;

//In main(int argc, char** argv) this global variable is initialized to argv[0]
const char* program_name;

//Initialized at the start of the program; gets the current time
auto start_time = std::chrono::high_resolution_clock::now();

//Returns the time since the start of the program, measured in seconds
//Has accuracy identical to that of std::chrono::high_resolution_clock
double age()
{
    return seconds_t(std::chrono::high_resolution_clock::now() - start_time).count();
}

//Prints the program usage
void PrintUsage()
{
    printf("Usage:\n");
    printf("%s [measurment duration]\n", program_name);
}
int main(int argc, char** argv)
{
    using namespace std::chrono_literals;
    program_name = argv[0];
    int sleeptime = 100;
    switch(argc) {
    case 1:
        sleeptime = 100;
        break;
    case 2:
        try
        {
            sleeptime = std::stoi(argv[1]);
        }
        catch(...)
        {
            printf("Error: argument was not an integer.");
            PrintUsage();
            return 1;
        }
        break;
    default:
        printf("Error: too many arguments.");
        PrintUsage();
        return 1;
        break;
    }
    uint64_t cycles_start = get_cycles();
    double time_start = age();
    std::this_thread::sleep_for(sleeptime * 1ms);
    uint64_t elapsed_cycles = get_cycles() - cycles_start;
    double elapsed_time = age() - time_start;
    printf("CPU MHz: %.3f\n", elapsed_cycles / elapsed_time / 1000000.0);
}

There are a number of things that you might employ to improve your program.

Accuracy

  Is there any reason the strategy used and described below won't produce accurate results?

Yes!  There are many reasons, which are each enumerated separately in items below, followed by more general information about code style and structure.

Understand System Management Mode

The code currently does not account for the possibilty of the processor entering System Management Mode (SMM).  One simple way of putting the machine into SMM is to have it go into standby.  My laptop is configured to go into standby when I close the lid and come back out when I open it again, so when I did that when running this program, I got this result:

  CPU MHz: 2365092677585.790

While it would be lovely to actually have a processor that fast, my laptop does not, so this result is incorrect.

Understand sleep_for

What std::this_thread::sleep_for(delay) does is to sleep for at least delay but it may be longer, depending on what else the operating system and the underlying hardware are doing at the moment.  This means that there is a variability in the results of the program.  An example of one thousand runs of the program is shown plotted below.

While in this case, most of the results were around the correct value of 2494.225 MHz, the results vary.  The alternate way to do this would be to use a hardware-based timer instead and run the timing code as part of the kernel with interrupts disabled.

Understand out-of-order execution and cache effects

The variability of this program as it currently stands is such that it doesn't matter, but when attempting to do precise timing using the RDTSC instruction, it's important to understand that the processor does out-of-order execution.  This means that instructions preceding or following the RDTSC instruction may actually be part of the duration of the event you're trying to measure unless precautions are taken.  What is generally used is a serializing instruction such as CPUID to eliminate such effects.  Intel has a useful whitepaper that describes this in detail and how to apply it.

General advice

What follows is more general advice about the coding style and structure.

Avoid global variables

The program_name and start_time variables are global variables.  It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.  If they must be globals (which is not the case here), then make them static.

Be careful with size assumptions

The code currently has these three lines:

unsigned int lo,hi;
__asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
return ((uint64_t)hi << 32) | lo;

It's not wrong as it is, but I'd probably write that using explicit uint32_t sizes for hi and lo to eliminate the possibility of a compiler with a 16-bit unsigned int.

Isolate code to a function

Right now, the timing is done in main.  I'd probably rewrite that to contain the timing code in a function:

double approx_CPU_MHz(unsigned sleeptime) {
    using namespace std::chrono_literals;
    uint64_t cycles_start = get_cycles();
    double time_start = age();
    std::this_thread::sleep_for(sleeptime * 1ms);
    uint64_t elapsed_cycles = get_cycles() - cycles_start;
    double elapsed_time = age() - time_start;
    return elapsed_cycles / elapsed_time / 1000000.0;
}

Think of the user

The PrintUsage routine looks like this:

void PrintUsage()
{
    printf("Usage:\n");
    printf("%s [measurment duration]\n", program_name);
}

There are three problems with that from the user's point of view.  First, it doesn't tell the user the unit of measure for the measurement duration.  Is it seconds?  Minutes?  Microseconds?  The user would have to look at the source code to answer that question.  Second, it could reasonably be interpreted to be two arguments, both of which are optional.  Putting an underscore instead of a space would fix that.  Third, the word "measurement" is misspelled.

Use iostream

It's not wrong to use printf but using iostreams is both more C++-like and can actually save some runtime processing.  To evaluate printf, the computer has to interpret the format string first, while using << means that the compiler has already evaluated the argument type at compile-time.


Find out max duplicate number between 1 to N numbers How can I make this method better?

int[] _input = new int[] { 5,45,184,594,14,8,5,184,1841,184,594,5,78,594,14,8,18,4,18,14,8,594};

Dictionary<int, int> _inputDictionary = new Dictionary<int, int>();

foreach (int i in _input)
{
    if (_inputDictionary.ContainsKey(i))
    {

        _inputDictionary[i] = ++_inputDictionary[i] ;

    }
    else
    {
        _inputDictionary.Add(i,1);
    }
}

Console.WriteLine();
Console.WriteLine("Maximum number repeated is " + _inputDictionary.OrderByDescending(g => g.Value).First().Key + " and it is repeated " + _inputDictionary.OrderByDescending(g => g.Value).First().Value+" times");

You can use GroupBy to count the duplicates.

Then you should sort the result and get the first item once, instead of sorting it each time you use it:

int[] _input = new int[] {
  5, 45, 184, 594, 14, 8, 5, 184, 1841, 184, 594, 5, 78, 594, 14, 8, 18, 4, 18, 14, 8, 594
};

IGrouping<int, int> max =
  _input.GroupBy(n => n)
  .OrderByDescending(g => g.Count())
  .First();

Console.WriteLine(
  "Maximum number repeated is " + max.Key +
  " and it is repeated " + max.Count() + " times"
);


How well or poorly structured are my routes in this NodeJS app? This is for an app built with Express. One of my concerns is the routes ending with / which I did because our previous site was an ASP app. So they named the folders that way for SEO purposes, then the page would be index.aspx .But in my case I just did, for example

science.jade and routed it from /science-bruxzir-zirconia-dental-crown/

Here is what I got for routes

app.js

var express = require('express');
var routes = require('./routes');
var user = require('./routes/user');
var http = require('http');
var path = require('path');

var app = express();

// all environments
app.set('port', process.env.PORT || 3000);
app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'jade');
app.use(express.favicon(path.join(__dirname, 'public/images/template/favicon.ico')));
app.use(express.logger('dev'));
app.use(express.json());
app.use(express.urlencoded());
app.use(express.methodOverride());
app.use(app.router);
app.use(require('stylus').middleware(path.join(__dirname, 'public')));
app.use(express.static(path.join(__dirname, 'public')));

// development only
if ('development' == app.get('env')) {
  app.use(express.errorHandler());
}

app.get('/', function(req, res){
  res.render('home', {
    title: 'Home'
  });
});

app.get('/features-bruxzir-zirconia-dental-crown/', function(req, res){
  res.render('features', {
    title: 'BruxZir Features'
  });
});

app.get('/science-bruxzir-zirconia-dental-crown/', function(req, res){
  res.render('science', {
    title: 'Scientific Validation'
  });
});

app.get('/video-bruxzir-zirconia-dental-crown/', function(req, res){
  res.render('videos', {
    title: 'BruxZir Video Gallery'
  });
});

app.get('/cases-bruxzir-zirconia-dental-crown/', function(req, res){
  res.render('cases', {
    title: 'Before & After Case Gallery'
  });
});

app.get('/testimonials-bruxzir-zirconia-dental-crown/', function(req, res){
  res.render('testimonials', {
    title: 'Bruxzir Testimonials'
  });
});

app.get('/authorized-bruxzir-labs-zirconia-dental-crown/', function(req, res){
  res.render('labs', {
    title: 'Authorized BruxZir Labs'
  });
});

app.get('/contact-bruxzir-zirconia/', function(req, res){
  res.render('contact', {
    title: 'Bruxzir Contact Form'
  });
});

app.get('/users', user.list);

http.createServer(app).listen(app.get('port'), function(){
  console.log('Express server listening on port ' + app.get('port'));
});

From a once over of the code and the comments:

This code is fine, especially if you have only a dozen routes, I would not advise you to build your own route building infrastructure for such a small amount of routes
Dont worry about the /
Consider having your routes in separate files as per @GnrlBzik


jQuery slideshow with connected links and the ability to pause/resume when hovered Update: Here is Take 2.

I want a slideshow with three primary pieces of functionality:

Has text links that act as bullet-points and can jump directly to a slide when clicked.
Pauses when a slide or link is moused over.
Resumes resumes playing from the currently active slide if the user clicks a link.

This is what I came up with. Thanks for your feedback.

$(function() {
  var slideCount = $("#carousel-slides > img").length
  var slideToActivate = 1
  var viewingLength = 2000
  var loop

  automateTheCarousel()

  /* === events handlers === */
  $("#carousel-nav").on("click", "a", function() {
    slideToActivate = $(this).data("linked-image")
    changeCarouselSlide()
  })

  $("#carousel-slides, #carousel-nav").hover(stopTheCarousel, automateTheCarousel)

  /* === functions === */
  function automateTheCarousel() {
    loop = setInterval(goToNextCarouselSlide, viewingLength)
  }

  function stopTheCarousel() {
    clearInterval(loop)
  }

  function goToNextCarouselSlide() {
    slideToActivate++
    if (slideToActivate > slideCount) {
      slideToActivate = 1
    }
    changeCarouselSlide()
  }

  function changeCarouselSlide() {
    // Deactivate everything.
    $("#carousel-slides img").hide()
    $("#carousel-nav a").css({
      "font-weight": "normal"
    })

    // Activate
    $("#" + slideToActivate).show()
    $("a[data-linked-image=" + slideToActivate + "]").css({
      "font-weight": "bold"
    })
  }

});
#carousel-slides {
  float: left;
  width: 50%;
}
#carousel-slides img {
  height: 200px;
  width: 200px;
  display: block;
  margin: 0 auto;
}
#carousel-nav {
  float: right;
  width: 50%;
  text-align: left;
}

  These are the tests!!!

  Test 1

  Test 2

  Test 3

And here is some more text.

One small note: the inline styling is there to visually set the first slide as "active" when the page loads. I feel like there's a better way, though.

Objects
Your code is fairly spread out and not very portable. All your variables are living in the top scope (well, not the global scope, but in your first $ call).
It would be more beneficial to pack this code into an object/class which keeps the state contained in the object itself. You could perhaps create something like this:
function ImageCarousel(slides, links) {
    this.slides = slides;
    this.links = links;

    // Here, you can setup the state of the carousel.
    this.slideToActivate = 1;
    this.viewingLength = 2000;
    this.loop = null;
}

With this design, nothing is leaking out into outer scopes and you can keep all the logic and state together.

Expensiveness
$ is an expensive function. It traverses the whole DOM every time you call it (or most of it), and this is certainly no speedy task.
$("#carousel-nav")

and
$("#carousel-slides...")

pop up more times than they should. Are the returns of these functions changing at any point in the program? No. Therefore, the results should be stored in variables
var navLinks = $("#carousel-nav");
var slides = $("#carousel-slides...");

Then, you can just access these variables rather than calling the function every time.

Simplify operation
slideToActivate++
if (slideToActivate > slideCount) {
  slideToActivate = 1
}
changeCarouselSlide()

can be simplified to
slideToActivate = (slideToActive + 1) % slideCount;
changeCarouselSlide()

The remainder operator (%) wraps numbers back around to 0 if they pass the right hand value.

Necessarianess
// Deactivate everything.

Is this really necessary? You have (or you should have) all the information you need to turn off the exact slides/links that are activated. For setups where there are a lot of slides and links, your current solution can get slow.
Well, we know the current slide number and we have an array of slides and links...
slides[slideToActivate].hide();
navLinks[slideToActivate].css(...);

ID's
Your current system of ID's is a little strange.
<img id="1"...
<img id="2"...
<img id="3"...

These ID's aren't descriptive at all of what the slides are. Here are some possible things you can do:

Change the ids to something like "imagecarousel-slide-1".
Use a data property like imagecarousel-slide="1".
Assume the slides are in the order in which they will appear.

The last method is certainly the easiest to implement. If you do choose one of the two former methods though, make sure that when you stick them in the array I mentioned above (navLinks or slides) you order them based on the property you decided.


Find the odd occurrences in an array Recently, I came across this problem on Codility. I am aware there is a bit of similarity to this question and this question, but I am trying to achieve this in C#.

The problem is as follows:

  A non-empty zero-indexed array \$A\$ consisting of \$N\$ integers is
  given. The array contains an odd number of elements, and each element
  of the array can be paired with another element that has the same
  value, except for one element that is left unpaired.

  For example, in array \$A\$ such that:

  \$\begin{align} & A[0] = 9, A[1] = 3, A[2] = 9 \\ & A[3] = 3, A[4] = 9, A[5] = 7 \\ & A[6] = 9 \end{align}\$

  The elements at indexes 0 and 2 have value 9
  The elements at indexes 1 and 3 have value 3
  The elements at indexes 4 and 6 have value 9
  The element at index 5 has value 7 and is unpaired

  Write a function:

class Solution { public int solution(int[] A); }

  that, given an array \$A\$ consisting of \$N\$ integers fulfilling the
  above conditions, returns the value of the unpaired element.

  For example, given array \$A\$ such that:

  \$\begin{align} & A[0] = 9, A[1] = 3, A[2] = 9 \\ & A[3] = 3, A[4] = 9, A[5] = 7 \\ & A[6] = 9 \end{align}\$

  the function should return 7, as explained in the example above.

  Assume that:

  \$N\$ is an odd integer within the range \$[1 \dots 1,000,000]\$
  Each element of array \$A\$ is an integer within the range \$[1 \dots 1,000,000,000]\$
  All but one of the values in \$A\$ occur an even number of times.

  Complexity:

  Expected worst-case time complexity is \$O(N)\$
  Expected worst-case space complexity is \$O(1)\$, beyond input storage (not counting the storage required for input arguments).
  Elements of input arrays can be modified.

I came up with a dictionary with the unique numbers as the key and the frequencies as the value and used Linq to filter the even numbers. Indirectly this means that a number divisible by 2 would have a pair. So far the code performs well, but I believe there is room for improvement as my performance was \$O(N)\$ and also out of curiosity how does Linq achieve better performance than the loops (e.g for).

    public static int OddOccurrencesInArray(int[] A)
    {
        Dictionary<int, int> dictionary = new Dictionary<int, int>();
        foreach(var item in A)
        {
            if (!dictionary.ContainsKey(item))
            {
                dictionary.Add(item, 1);
            }
            else
            {
                dictionary[item]++;
            }
        }
       var query = dictionary.Where(k=>(k.Value % 2) != 0).ToDictionary(k=>k.Key, k=>k.Value);

        foreach(var p in query)
        {
            return p.Key;
        }

        return 0;
    }
    static void Main(string[] args)
    {
        int[] A = { 9, 3, 9, 3, 9, 7, 9};
        Console.WriteLine(OddOccurrencesInArray(A));
        Console.ReadLine();
    }
}

There is a simple solution that does not require a dictionary:

int[] A = { 9, 3, 9, 3, 9, 7, 9};
int result = 0;
foreach (var i in A)
    result ^= i;
Console.WriteLine(result); // 7

The trick here is, that "a xor a" gives 0. So all number will be erased except the single one.

Or in short:

var result = new [] { 9, 3, 9, 3, 9, 7, 9}.Aggregate(0, (a, b) => a^b);

To you solution with the dictionary:

You don't need a dictionary. You could just use a Hashset. Add a new entry if the entry does not exists and remove the entry if it already exists. After processing all items, the hashset should have one item - the result.


Iterating over a range of integer ranges in Java My use case is this:

RangeDemo.java:

package net.coderodde.util;

public class RangeDemo {

    public static void main(String[] args) {
        for (Integer i : new Ranges(new Range(-9, 0),
                                    new Range(3, 5),
                                    new Range(15, 10),
                                    new Range(11, 11),
                                    new Range(16, 17))) {
            System.out.print(i + " ");
        }
    }
}

... which gives me:

-9 -8 -7 -6 -5 -4 -3 -2 -1 3 4 10 11 12 13 14 16

My implementation is this:

Range.java:

package net.coderodde.util;

/**
 * This class implements a range of integers.
 *
 * @author Rodion "rodde" EfremoB.
 * @version 1.6 (Jun 10, 2019)
 */
public final class Range {

    private final int fromIndex;
    private final int toIndex;

    public Range(final int fromIndex, final int toIndex) {
        this.fromIndex = Math.min(fromIndex, toIndex);
        this.toIndex   = Math.max(fromIndex, toIndex);
    }

    int get(int index) {
        return fromIndex + index;
    }

    int size() {
        return toIndex - fromIndex;
    }
}

Ranges.java:

package net.coderodde.util;

import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.function.Consumer;

/**
 * This class implements an integer iterator over a range of ranges.
 *
 * @author rodde
 */
public final class Ranges implements Iterable<Integer> {

    /**
     * This static inner class implements the actual iterator.
     */
    private static final class RangeIterator implements Iterator<Integer> {

        /**
         * Holds the actual ranges.
         */
        private final Range[] ranges;

        /**
         * The index of the current range.
         */
        private int currentRangeIndex;

        /**
         * The current index within the current range.
         */
        private int currentElementIndex;

        /**
         * The total number of integers to iterate.
         */
        private final int globalSize;

        /**
         * The global number of integers already iterated.
         */
        private int globalIndex;

        private RangeIterator(Range[] ranges) {
            this.ranges = ranges;
            this.globalSize = countGlobalSize();

        }

        @Override
        public boolean hasNext() {
            return globalIndex < globalSize;
        }

        @Override
        public Integer next() {
            if (!hasNext()) {
                throw new NoSuchElementException("Nothing to iterate.");
            }

            while (currentElementIndex == ranges[currentRangeIndex].size()) {
                currentRangeIndex++;
                currentElementIndex = 0;
            }

            globalIndex++;
            return ranges[currentRangeIndex].get(currentElementIndex++);
        }

        private int countGlobalSize() {
            int globalSize = 0;

            for (Range r : ranges) {
                globalSize += r.size();
            }

            return globalSize;
        }
    }

    private final Range[] ranges;

    public Ranges(Range... ranges) {
        this.ranges = Objects.requireNonNull(ranges);
    }

    @Override
    public Iterator<Integer> iterator() {
        return new RangeIterator(ranges);
    }

    @Override
    public void forEach(Consumer<? super Integer> action) {
        RangeIterator rangeIterator = new RangeIterator(this.ranges);
        rangeIterator.forEachRemaining(action);
    }
}

I think RangeCollection would be a better name than Ranges.

To my mind, the RangeIterator is doing too much.  It's iterating over a collection of ranges and over each range in the collection.  To me it would make more sense for the Range class to have its own iterator and the RangeCollection its own iterator


Unwinding shortened URLs with Python, SQLite, and multithreading I spent the last couple of days reading/researching and learning sqlite and python multithreading.

I'm unwinding shortened urls stored in a sql table with three fields...
id | url_starting | url_unwound

The code chunks the database results, sends the chunk to a thread pool of the same chunk size, unwinds the urls and writes the results back to the database, then iterates to the next chunk.

I'm happy with the results but am looking for feedback as this is my first attempt.

General thoughts are greatly appreciated.
Also, any ideas on making the future_to_url = section more readable would be great.

import time
import os
import sqlite3
from datetime import datetime
from concurrent.futures import ThreadPoolExecutor, as_completed
from urllib.request import Request, urlopen

class MyDatabase(object):
    def __init__(self, db):
        self.__conn = sqlite3.connect(db)
        self.__conn.row_factory = sqlite3.Row  # https://stackoverflow.com/a/41920171/25197
        self.__conn.execute('pragma foreign_keys = on')
        self.__conn.commit()
        self.__cursor = self.__conn.cursor()

    def __enter__(self):
        return self

    def __del__(self):
        self.__conn.close()

    def __exit__(self, exc_type, exc_value, traceback):
        self.__conn.close()

    def query(self, arg):
        self.__cursor.execute(arg)
        self.__conn.commit()
        return self.__cursor

    def updateUnwoundUrl(self, url_starting, url_unwound):
        self.__cursor.execute(
            'UPDATE url SET url_unwound = ? WHERE url_starting = ?', (url_unwound, url_starting))
        self.__conn.commit()
        return self.__cursor

def unwindUrl(starting_url, timeout):
    try:
        hdr = {'User-Agent': 'Mozilla/5.0 (Windows NT 6.1; Win64; x64)'}
        response = urlopen(Request(starting_url, headers=hdr), timeout=timeout)
        unwound_url = response.geturl()

    # For any http error just return error. Deal with specifics later.
    except Exception:
        unwound_url = 'error'

    return unwound_url

def chunk_iter(iterable, chunk_size):
    it = iter(iterable)
    while True:
        chunk = tuple(next(it) for _ in range(chunk_size))
        if not chunk:
            break
        yield chunk

def unwindUrls(workers=100, request_timeout=10):
    relative_path = os.path.dirname(__file__)
    my_db_path = os.path.join(relative_path, 'database', 'my.db')

    with MyDatabase(my_db_path) as my_db:
        wound_urls = my_db.query(
            'SELECT url_starting, id FROM url WHERE url_unwound IS NULL ORDER BY id ASC').fetchall()

        chunk_count = len(wound_urls) / workers
        for i, wound_urls_chunk in enumerate(chunk_iter(wound_urls, workers), start=1):

            print('starting chunk {0} of {1} at {2}'.format(
                i, chunk_count, datetime.now().strftime("%I:%M:%S %p")))

            with ThreadPoolExecutor(max_workers=workers) as executor:
                future_to_url = {executor.submit(
                    unwindUrl, row['url_starting'], request_timeout): row['url_starting'] for row in wound_urls_chunk}

                for future in as_completed(future_to_url):
                    try:
                        url_starting = future_to_url[future]
                        url_unwound = future.result()
                        my_db.updateUnwoundUrl(url_starting, url_unwound)
                        # print('starting: {0} unwound: {1}'.format(url_starting, url_unwound))
                    except Exception as exc:
                        print('!! {0}'.format(exc))

if __name__ == '__main__':
    startTime = time.time()
    unwindUrls()
    print('==--- unwind_urls took {0} seconds. ---=='.format(round(time.time() - startTime)))

__

There is liitle need for the __. A single _ should suffice to make people clear it's a private attribute. If they really want to use it, __ won't stop the access

Linelength

Especially the lines with queries are too long. Can't you make those queries multiline?

From:

def updateUnwoundUrl(self, url_starting, url_unwound):
    self.__cursor.execute(
        'UPDATE url SET url_unwound = ? WHERE url_starting = ?', (url_unwound, url_starting))
    self.__conn.commit()
    return self.__cursor

to:

def updateUnwoundUrl(self, url_starting, url_unwound):
    self.__cursor.execute(
        '''UPDATE url
           SET url_unwound = ?
           WHERE url_starting = ?
        '''
        , (url_unwound, url_starting)
    )
    self.__conn.commit()
    return self.__cursor

Hoist the IO

If ever you want to change the db format to something else, you will need to change the internals of the code and how the my_db_path is made.

To change this, I would follow the tips from this talk.

Database path

def unwindUrls(workers=100, request_timeout=10):
    relative_path = os.path.dirname(__file__)
    my_db_path = os.path.join(relative_path, 'database', 'my.db')

First, I would use my_db_path to handle filepaths.

Secondly I would add relative_path as an an argument, with Path('.') as default argument, this way you can specify the path.

def unwindUrls(workers=100, request_timeout=10):
    relative_path = os.path.dirname(__file__)
    my_db_path = os.path.join(relative_path, 'database', 'my.db')
    with MyDatabase(my_db_path) as my_db:

...
if __name__ == '__main__':
    unwindUrls()

To:

def unwindUrls(my_db, workers=100, request_timeout=10):

...
if __name__ == '__main__':
    my_db_path = pathlib.Path('.') / 'database' / 'my.db'
    with MyDatabase(my_db_path) as my_db:
        unwindUrls(my_db)

chunk_iter

def chunk_iter(iterable, chunk_size):
    it = iter(iterable)
    while True:
        chunk = tuple(next(it) for _ in range(chunk_size))
        if not chunk:
            break
        yield chunk

raises a Warning in Python 3.6

  C:\miniconda3\envs\test_env\lib\site-packages\ipykernel_launcher.py:4:
  DeprecationWarning: generator 'chunk_iter..' raised
  StopIteration   after removing the cwd from sys.path.

Better to use itertools.islice

def chunk_iter(iterable, chunk_size):
    it = iter(iterable)
    while True:
        chunk = tuple(islice(it, chunk_size))
        if not chunk:
            return
        yield chunk

Exception

a few time you use except Exception. This is way too broad. You should specify which Exceptions you want to catch


Animation of adding / removing nodes from VBox Method hide_old_show_new (I can't think of a suitable name) gradually, using FadeTransition, removes all nodes (from last to first) and then adds new nodes (from first to last)

But, it is unnecessarily complex.

Do you know how to simplify it? Or a better algorithm? I'll be grateful for the improvement.

However, I do not want to use an algorithm using *.fxml.

import java.util.ArrayList;
import javafx.animation.FadeTransition;
import javafx.scene.text.Text;
import javafx.util.Duration;

public final class Cart extends javafx.scene.layout.VBox {

    public Cart(double x, double y, double width, double height) {
        super();
        setLayoutX(x);
        setLayoutY(y);
        setWidth(width);
        setHeight(height);
    }

    public FadeTransition hide_old_show_new(ArrayList<Text> nodes) {
        var transition = new FadeTransition();
        var size = getChildren().size();
        if (size > 0 && !getChildren().get(0).equals(nodes.get(0))) {
            transition = new FadeTransition(Duration.millis(200), getChildren().get(size - 1));
            transition.setFromValue(1);
            transition.setToValue(0);
            transition.setOnFinished((event) -> {
                getChildren().remove(size - 1);
                hide_old_show_new(nodes);
            });
            transition.play();
        } else if (size < nodes.size()) {
            getChildren().add(nodes.get(size));
            getChildren().get(size).setOpacity(0);
            transition = new FadeTransition(Duration.millis(200), getChildren().get(size));
            transition.setFromValue(0);
            transition.setToValue(1);
            transition.setOnFinished((event) -> {
                hide_old_show_new(nodes);
            });
            transition.play();
        }
        return transition;
    }
}

So I divided it into two methods. This made the conditions considerably easier.

Still, it doesn't seem perfect to me. Plus, I found a mistake. If I run a new animation, an error occurs if the old animation has not been completed yet.

But I don't know how to treat it.

Do you have any idea how, please?

Thank you

public FadeTransition hide_old(ArrayList<Text> nodes) {
    var transition = new FadeTransition();
    var size = getChildren().size();
    if (size > 0) {
        transition = new FadeTransition(Duration.millis(200), getChildren().get(size - 1));
        transition.setFromValue(1);
        transition.setToValue(0);
        transition.setOnFinished((event) -> {
            getChildren().remove(size - 1);
            hide_old(nodes);
        });
        transition.play();
    } else {
        show_new(nodes);
    }
    return transition;
}

private void show_new(ArrayList<Text> nodes) {
    FadeTransition transition;
    var size = getChildren().size();
    if (size < nodes.size()) {
        getChildren().add(nodes.get(size));
        getChildren().get(size).setOpacity(0);
        transition = new FadeTransition(Duration.millis(200), getChildren().get(size));
        transition.setFromValue(0);
        transition.setToValue(1);
        transition.setOnFinished((event) -> {
            show_new(nodes);
        });
        transition.play();
    }
}


Merge two integer lists in Pascal The purpose of the code is to merge two ordered lists (list 1 : 5 integers, list 2 : 8 integers) into another third ordered list, whilst maintaining their order, while entries that appear in more than one list also appear multiple times in the result.

Example:

An input of List1 (Feld1): 3 6 7 9 10, merged with an input of List2(Feld2): 2 7 11 12 15 17 20 21 would result into a third list(Ergebnisfeld) of:
2 3 6 7 7 9 10 11 12 15 17 20 21.

This code works well on my computer, but since I am pretty new to this, I would like to ask, if this is:

The most efficient way to do this (e.g. is the combination of a while and a repeat/until loop the best code for this, or is a for loop better)
If there are any problems with the code, which I am too inexperienced to see, but which might cause me problems later

 program merge (input, output);
    { Reads two ordered lists of integers keyed in by the user, and merges
them into a third ordered list, without checking their order.
    if the input is not sorted, the result will be undefined.}

const
  FELDLAENGE1 = 5;
  FELDLAENGE2 = 8;
  ERGEBNISFELDLAENGE = 13; { FELDLAENGE1 + FELDLAENGE2}

type
  tFeld1 = array [1..FELDLAENGE1] of integer;
  tFeld2 = array [1..FELDLAENGE2] of integer;
  tErgebnisFeld = array [ 1..ERGEBNISFELDLAENGE] of integer;

var
  Feld1 : tFeld1;
  Feld2 : tFeld2;
  ErgebnisFeld : tErgebnisFeld;
  i : integer;
  j : integer;
  k : integer;
begin
  { read ordered lists (Felder) }
  writeln (' Please key in', FELDLAENGE1:2, ' values in ascending order for
the first list. ');
  for i := 1 to FELDLAENGE1 do
      readln (Feld1[i]);
  writeln (' Please key in', FELDLAENGE2:2, ' values in ascending order for
the second list. ');
  for j := 1 to FELDLAENGE2 do
      readln (Feld2[j]);
  { Merge Feld1 and Feld2 into ErgebnisFeld}
  i :=1;
  j :=1;
  k := 1;

  while (i <= FELDLAENGE1) and (j <= FELDLAENGE2) do
        if Feld1[i] <= Feld2[j] then
           begin
           ErgebnisFeld[k] := Feld1[i];
           i := i+1;
           k := k+1
          end
        else
        begin
          ErgebnisFeld[k] := Feld2[j];
          j := j +1;
          k := k +1
        end;
  { add remaining values to ErgebnisFeld}
  if i > FELDLAENGE1 then
     repeat
           ErgebnisFeld[k] := Feld2[j];
           j := j + 1;
           k := k + 1
     until k = ERGEBNISFELDLAENGE+1
  else
      repeat
            ErgebnisFeld[k] := Feld1[i];
            i := i + 1;
            k := k + 1
      until k = ERGEBNISFELDLAENGE+1;

  writeln ('The merged list is: ');
  for k := 1 to ERGEBNISFELDLAENGE do
      write (ErgebnisFeld[k], ' ');
  writeln;
  readln;

end.

First, I think you might separate the input/output part from processing data by enclosing the merging routine in a separate procedure. Then the merging procedure can be reused in many places of your program. And if you move it to a separate unit you'll be able to reuse it in other programs, too, if necessary.

 program merge (input, output);
    { Reads two ordered lists of integers keyed in by the user, and merges
    them into a third ordered list, without checking their order.
    If the input is not sorted, the result will be undefined.}

const
  FELDLAENGE1 = 5;
  FELDLAENGE2 = 8;
  ERGEBNISFELDLAENGE = 13; { FELDLAENGE1 + FELDLAENGE2}

type
  tFeld1 = array [1..FELDLAENGE1] of integer;
  tFeld2 = array [1..FELDLAENGE2] of integer;
  tErgebnisFeld = array [ 1..ERGEBNISFELDLAENGE] of integer;

procedure mergeTwoArrays( var Feld1 : tFeld1; var Feld2 : tFeld2;
                          var ErgebnisFeld : tErgebnisFeld)
    var
        i : integer;
        j : integer;
        k : integer;
    begin
        i := 1;
        j := 1;
        k := 1;

        while (i <= FELDLAENGE1) and (j <= FELDLAENGE2) do
            if Feld1[i] <= Feld2[j] then
              begin
                ErgebnisFeld[k] := Feld1[i];
                i := i + 1;
                k := k + 1
              end
            else
              begin
                ErgebnisFeld[k] := Feld2[j];
                j := j + 1;
                k := k + 1
              end;
        { add remaining values to ErgebnisFeld}
        if i > FELDLAENGE1 then
            repeat
               ErgebnisFeld[k] := Feld2[j];
               j := j + 1;
               k := k + 1
            until k = ERGEBNISFELDLAENGE+1
        else
            repeat
                ErgebnisFeld[k] := Feld1[i];
                i := i + 1;
                k := k + 1
            until k = ERGEBNISFELDLAENGE+1;
    end;

var
  Feld1 : tFeld1;
  Feld2 : tFeld2;
  ErgebnisFeld : tErgebnisFeld;
  i : integer;
  j : integer;
begin
  { read ordered lists (Felder) }
  writeln (' Please key in', FELDLAENGE1:2, ' values in ascending order for the first list. ');
  for i := 1 to FELDLAENGE1 do
      readln (Feld1[i]);
  writeln (' Please key in', FELDLAENGE2:2, ' values in ascending order for the second list. ');
  for j := 1 to FELDLAENGE2 do
      readln (Feld2[j]);

  { Merge Feld1 and Feld2 into ErgebnisFeld}
  mergeTwoArrays( Feld1, Feld2, ErgebnisFeld);

  { output results from ErgebnisFeld}
  writeln ('The merged list is: ');
  for k := 1 to ERGEBNISFELDLAENGE do
      write (ErgebnisFeld[k], ' ');
  writeln;
  readln;

end.

Another change I would make is simplifying the code to append input lists' tails in the final part of merge – you can do that with two separate while loops, similar to the main merging loop. Exactly one of them will do the actual work, and the cost of not entering the other one equals the cost of the conditional (if-then-else) in your version.

      { add remaining values from Feld1 to ErgebnisFeld}
      while i <= FELDLAENGE1 do
          begin
              ErgebnisFeld[k] := Feld1[i];
              i := i + 1;
              k := k + 1
          end;

      { add remaining values from Feld2 to ErgebnisFeld}
      while j <= FELDLAENGE2 do
          begin
              ErgebnisFeld[k] := Feld2[j];
              j := j + 1;
              k := k + 1
          end;

Then you may use open arrays to make the routine more generic, so it could merge arrays of arbitrary lenghts, not just 5 plus 8.

You can also explicitly pass the lengths of both input lists as parameters to the routine, so it can merge lists shorter than arrays in which they're stored.

procedure mergeTwoArrays( var Feld1 : array of integer; feldLange1 : integer;
                          var Feld2 : array of integer; feldLange2 : integer;
                          var ErgebnisFeld : array of integer)
    var
        i, j, k : integer;
    begin
        i := 0;    { indices of open arrays start from 0 }
        j := 0;    { and end at length-1 }
        k := 0;

        { merge Feld1 and Feld2 to ErgebnisFeld until one of them becomes empty}
        while (i < feldLange1) and (j < feldLange2) do { note the comp. operator }
            if Feld1[i] <= Feld2[j] then
              begin
                ErgebnisFeld[k] := Feld1[i];
                i := i + 1;
                k := k + 1
              end
            else
              begin
                ErgebnisFeld[k] := Feld2[j];
                j := j + 1;
                k := k + 1
              end;

        { add remaining values from Feld1 to ErgebnisFeld}
        while i < feldLange1 do
            begin
                ErgebnisFeld[k] := Feld1[i];
                i := i + 1;
                k := k + 1
            end;

        { add remaining values from Feld2 to ErgebnisFeld}
        while j < feldLange2 do
            begin
                ErgebnisFeld[k] := Feld2[j];
                j := j + 1;
                k := k + 1
            end;
    end;


Codewars: Reverse or Rotate CodeWars 6 Kyu: Reverse or rotate?

I wrote my Javascript "hello world" on Thursday, so I'm definitely a JS beginner. I do have some experience in Python though, so I'm not a complete novice to programming in general.

Question

  The input is a string str of digits. Cut the string into chunks (a chunk here is a substring of the initial string) of size sz (ignore the last chunk if its size is less than sz).

  If a chunk represents an integer such as the sum of the cubes of its digits is divisible by 2, reverse that chunk; otherwise rotate it to the left by one position. Put together these modified chunks and return the result as a string.

  If

  sz is <= 0 or if str is empty return ""
  sz is greater (>) than the length of str it is impossible to take a chunk of size sz hence return "".

  Examples:

revrot("123456987654", 6) --> "234561876549"
revrot("123456987653", 6) --> "234561356789"
revrot("66443875", 4) --> "44668753"
revrot("66443875", 8) --> "64438756"
revrot("664438769", 8) --> "67834466"
revrot("123456779", 8) --> "23456771"
revrot("", 8) --> ""
revrot("123456779", 0) --> ""
revrot("563000655734469485", 4) --> "0365065073456944"

Solution

function revrot(str, sz)
{
   ln = str.length;
   if(sz < 1 || !str || sz > ln) return "";

   const test = s => Array.prototype.reduce.call(s, (acc, val) => acc + Number(val) ** 3, 0) % 2 === 0;
   const reverse = s => s.split("").reverse().join("");
   const rotate = s => s.slice(1) + s.slice(0, 1);

   let arr = [];
   for(let i = 0; i < ln; i += sz) arr.push(i+sz <= ln ? str.slice(i, i+sz) : "")
   return arr.map(x => test(x) ? reverse(x) : rotate(x)).join("");
}

Towards optimization and restructuring

One of downsides of the initial revrot approach is that all nested functions test, reverse and rotate will be redundantly recreated on each revrot call.Instead - define them as top-level functions with meaningful names.

Next, the program lacks 2 edge cases:

when input string contains one single digit and size of required chunk is also 1 (ex. revrot("3", 1)). It's worth to catch that case immediately and return the unchanged input string
when input string contains non-digit character

The intermediate arr array with for loop and slice calls (to get all chunks) is replaced with convenient String.match call on dynamic regex pattern new RegExp(`.{${size}}`, 'g') (matches substrings of specific length/size)

The final optimized approach:

const testCubes = s => Array.prototype.reduce.call(s, (acc, val) => acc + Number(val) ** 3, 0) % 2 === 0;
const strReverse = s => [...s].reverse().join("");
const rotateLeft = s => s.slice(1) + s.slice(0, 1);

function revrot(str, size) {
    let len = str.length;
    if (size < 1 || !str || size > len) return "";
    if (len === 1 && size === 1) return str;
    if (/\D/.test(str)) {
        throw Error("Invalid numeric string");
    }
    let pat = new RegExp(`.{${size}}`, 'g');
    return str.match(pat).map(x =>
        testCubes(x) ? strReverse(x) : rotateLeft(x)
    ).join("");
}

console.log(revrot("123456987654", 6));
console.log(revrot("123456987653", 6));
console.log(revrot("66443875", 8));
console.log(revrot("563000655734469485", 4));
console.log(revrot("1", 1));
console.log(revrot("6644-d3875", 8));


HackerRank: Fraudulent activity notification I wanted to get some constructive feedback on my solution to the Fraudulent Activity Notification problem from HackerRank:

  HackerLand National Bank has a simple policy for warning clients about possible fraudulent account activity. If the amount spent by a client on a particular day is greater than or equal to 2× the client's median spending for a trailing number of days, , they send the client a notification about potential fraud. The bank doesn't send the client any notifications until they have at least that trailing number of prior days' transaction data.

  Given the number of trailing days d and a client's total daily expenditures for a period of n days, find and print the number of times the client will receive a notification over all n days.

  For example, d = 3 and expenditures = [10, 20, 30, 40, 50]. On the first three days, they just collect spending data. At day 4, we have trailing expenditures of [10, 20, 30]. The median is 20 and the day's expenditure is 40. Because 40 ≥ 2 × 20, there will be a notice. The next day, our trailing expenditures are [20, 30, 40] and the expenditures are 50. This is less than 2 × 30 so no notice will be sent. Over the period, there was one notice sent.

  Input Format

  The first line contains two space-separated integers n and d, the number of days of transaction data, and the number of trailing days' data used to calculate median spending.
  The second line contains n space-separated non-negative integers where each integer i denotes expenditure[i].

  Constraints

  1 ≤ n ≤ 2×105
  1 ≤ d ≤ n
  0 ≤ expenditure[i] ≤ 200

I believe having the a sort in there in raising the timeout for some problems.
How would you solve it without a sort? Sorting while inserting has a much higher complexity...

#include <bits/stdc++.h>
#include <array>

using namespace std;

vector<string> split_string(string);

// Complete the activityNotifications function below.
int activityNotifications(vector<int> expenditure, int d) {
  int result=0;
  int dq_idx=0;
  double median=0;
  vector<int> dq;
  for (int i =0; i< expenditure.size()-1; i++){
    if (dq_idx >= dq.size()){
      dq.push_back(expenditure[i]);
    }else {
      dq.at(dq_idx) = expenditure[i];
    }
    dq_idx=(dq_idx+1) %d;

    if (dq.size()>=d){
      sort(dq.begin(), dq.end());
      if (d %2 ==0){
        median = 2* (dq[d/2 -1 ] + dq[(d/2)])/2;
      }else {
        median = 2 * dq[d%2];
      }
      if ((float)expenditure[i+1] >= median) {
        result++;
      }
    }
  }
  return result;
}

int main()
{
    ofstream fout(getenv("OUTPUT_PATH"));

    string nd_temp;
    getline(cin, nd_temp);

    vector<string> nd = split_string(nd_temp);

    int n = stoi(nd[0]);

    int d = stoi(nd[1]);

    string expenditure_temp_temp;
    getline(cin, expenditure_temp_temp);

    vector<string> expenditure_temp = split_string(expenditure_temp_temp);

    vector<int> expenditure(n);

    for (int i = 0; i < n; i++) {
        int expenditure_item = stoi(expenditure_temp[i]);

        expenditure[i] = expenditure_item;
    }

    int result = activityNotifications(expenditure, d);

    fout << result << "\n";

    fout.close();

    return 0;
}

vector<string> split_string(string input_string) {
    string::iterator new_end = unique(input_string.begin(), input_string.end(), [] (const char &x, const char &y) {
        return x == y and x == ' ';
    });

    input_string.erase(new_end, input_string.end());

    while (input_string[input_string.length() - 1] == ' ') {
        input_string.pop_back();
    }

    vector<string> splits;
    char delimiter = ' ';

    size_t i = 0;
    size_t pos = input_string.find(delimiter);

    while (pos != string::npos) {
        splits.push_back(input_string.substr(i, pos - i));

        i = pos + 1;
        pos = input_string.find(delimiter, i);
    }

    splits.push_back(input_string.substr(i, min(pos, input_string.length()) - i + 1));

    return splits;
}

Bad Habits
I'd get out of the habit of including bits/stdc++.h as it's non-standard and makes your compile times much longer than they need to be. Especially with such a short program.
Likewise, using namespace std can lead to problems and should be avoided.
Naming
You should use clearer variable and function names. The function name activityNotifications() sounds like it could be presenting the user with notifications. Or it could be processing activity notifications. Instead it's counting them. It might be better named countActivityNotifications().
Within the function, you have a few good variable names. Namely, median and results. I have no idea what dq stands for, which means I also have no idea what dq_idx stands for (beyond that it's an index for dq). expenditure is singular, but it holds multiple expenditures, so I'd make the name plural. d is unhelpful as well. Just because the problem description named the variables n and d doesn't mean your code should use those names. They should be numberOfExpenditureDays and numberOfTrailingDays. (If you prefer num to number and removing Of, that's fine.)
Simplify
You don't need to sort the expenditures. You can get the median by generating a histogram and finding the middle value. A histogram is just an array where each element is the number of times that value was in the input. So a histogram of expenditures would hold the count of how many expenditures were for 1 dollar, and 2 dollars, and 3 dollars, etc. Since the expenditures are between 0 and 200 dollars each, you would have 200 buckets in your histogram. Furthermore, since you're dealing with trailing days, once you have the first one calculated, you can simply update 2 values on each subsequent pass. (Decrement the expenditure from the day that's falling off the array and increment the one that's being added in.)
Your split_string() function goes to a lot of trouble to clean the string before you tokenize it. But you don't need to. You can simply not add empty strings to the results. Something like this:
void split(const std::string& s, std::vector<std::string>& results)
{
    size_t lastPos = 0;
    size_t nextPos = 0;
    while ((nextPos = s.find(" ", lastPos)) != std::string::npos)
    {
        std::string word = s.substr(lastPos, nextPos - lastPos);
        if (!word.empty()) {
            results.push_back(word);
        }
        lastPos = nextPos + 1;
    }

    if (lastPos != std::string::npos)
    {
        std::string word = s.substr(lastPos, std::string::npos);
        if (!word.empty())
        {
            results.push_back(word);
        }
    }
}

I had to read your function several times to realize what it was trying to do with std::unique(), std::string::erase(), and std::string::pop_back().
Readability
The activityNotifications() function is really hard to read due to the odd, inconsistent spacing it uses. I recommend a space before and after every operator to make it more clear. So this line, for example:
dq_idx=(dq_idx+1) %d;

is more readable when written like this:
dq_idx = (dq_idx + 1) % d;