Find the Sum of Digit XOR till N? The question is to find the sum of Digit XOR till \$N\$.

For eg:-
Digit XOR of a number 112

  112 => 1 xor 1 xor 2 =  2

So we need to find

int digit_xor_sum=0
for(i=1;i<=N;i++){
  digit_xor_sum+= digit_xor(i);
}
return digit_xor_sum

My Solution Approach was as follows,

 int digit_xor(int n){
    if(n<=9){
        return n; // because there is only 1 digit
    }

    int digit_xor_sol=(n%10); // intialising digit_xor_sol with last digit
    n/=10; // removing the last digit

    while(n){
        digit_xor_sol = digit_xor_sol ^ (n%10);  // xor the current last digit and current xor solution
        n/=10;
    }

    return digit_xor_sol;
}

But this is \$O(nlogn)\$, which is time expensive.

Then I found a better solution like the below, which takes \$O(n)\$:

int digit_xor[1000];

int find_digit_xor(int n){
    if(n<=9)
        return digit_xor[n]=n;

    if(digit_xor[n]!=-1){ // there are multiple test-cases, so no need to recompute again
       return digit_xor[n];
    }
    return digit_xor[n] = digit_xor[n/10]^(n%10);
}

int main(){

 int sum;
 memset(digit_xor,-1,sizeof digit_xor);

 int testcase;
 scanf("%d",&testcase);

 while(testcase--){
     int n;
     scanf("%d",&n);

     sum=0;

     for(int i=0;i<=n;i++)
         sum+=find_digit_xor(i);

     printf("%d\n",sum);
 }
 return 0;
}

But this too is time expensive for larger \$N\$.

Is there any direct formula to calculate this SUM in \$O(1)\$.

Updated review

Peter Taylor noticed that I was solving for the wrong problem.  The problem calls for a sum of xor sums, not a single xor sum.  So I'm updating a review to talk about the actual problem (the old review is at the end, for reference).

Current solution is \$O(n)\$, is it enough?

The current solution with memoization runs in \$O(n)\$ time.  I was able to run with n = 200000000 (200 million) in under one second.  The question did not specify how many test cases there would be and how large \$n\$ could be for each testcase.  But since the OP said it wasn't fast enough, I'm going to assume that \$n\$ could be very large.  In that case, both time and space become a concern.

Can be solved in \$O(\log n)\$ time and space

Peter Taylor's answer outlined a solution where you solve for each of four bits (1,2,4,8) separately and then add up to a final total.  This solution requires \$O(\log n)\$ time due to its recursive nature, but does not use any extra space other than the \$O(\log n)\$ space needed to handle the recursion.

I got interested in how difficult it would be to implement this solution, so I did it.  I also tested versus the memoized version to make sure it worked.  Here is the result (quite long due to the number of comments):

#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>

#define MAX_DIGITS        20

const uint8_t ones[]   = { 1, 3, 5, 7, 9 };
const uint8_t twos[]   = { 2, 3, 6, 7 };
const uint8_t fours[]  = { 4, 5, 6, 7 };
const uint8_t eights[] = { 8, 9 };

// Returns true if "digit" is in the given set of digits "digitSet".
// For example, 3 is in the set { 1, 3, 5, 7, 9 }.
static bool isInSet(int digit, const uint8_t *digitSet, int numDigitSet)
{
    int i = 0;

    for (i=0;i<numDigitSet;i++) {
        if (digitSet[i] == digit)
            return 1;
    }
    return 0;
}

// Returns (n choose k), which is the number of ways to choose k objects from
// a set of n objects.
static uint64_t combinations(int n, int k)
{
    int      i   = 0;
    uint64_t ret = 1;

    // nCk equals nC(n-k), so solve for the easier case.
    if (n-k < k)
        k = n-k;

    for (i=0;i<k;i++) {
        ret *= (n-i);
        ret /= (i+1);
    }
    return ret;
}

// Given every possible number of numDigits length, return the number of ways
// that you can make a number with an odd/even number of digits coming from
// a set of size numInSet.  For example:
//
// 5 digits
// 4 numbers in the set
// odd
//
// Let D be a digit in the set and x be a digit not in the set.
//
// Since we are considering only odd possibilities, we can make numbers with
// either 1 D, 3 Ds, or 5 Ds, like this:
//
// Dxxxx xDxxx xxDxx xxxDx xxxxD = 1 D  (5 choose 1 =  5 ways to place)
// DDDxx DDxDx DDxxD DxDDx etc.  = 3 Ds (5 choose 3 = 10 ways to place)
// DDDDD                         = 5 Ds (5 choose 5 =  1 way  to place)
//
// Within each of the ways above, such as Dxxxx, there are also many ways
// to build the digits.  There are numInSet digits (4 in the example) that
// can be put in each D slot, and (10 - numInSet) digits (6 in the example)
// ways to fill each x slot.  So for Dxxxx, there are 4*6*6*6*6 ways to create
// a matching number of that pattern.  For DDDxx, there are 4*4*4*6*6 ways.
// For DDDDD there are 4*4*4*4*4 ways.
//
// So the answer for the example above would be:
//
// 5*(4*6*6*6*6) + 10*(4*4*4*6*6) + 1*(4*4*4*4*4) = 49984 ways
//
static uint64_t computeCount(int numDigits, int numInSet, bool odd)
{
    int      i            = (odd ? 1 : 0);
    int      j            = 0;
    uint64_t ret          = 0;
    int      numNotInSet  = 10 - numInSet;
    uint64_t waysPerCombo = 1;

    // Special case for 0 digits.
    if (numDigits == 0)
        return odd ? 0 : 1;

    // Preinitialize waysPerCombo so we don't have to recompute the whole
    // thing every time through the next loop.  We will only need to adjust
    // it for 2 digits every loop iteration.  This is the 4*6*6*6*6 factor
    // from the example above.
    for (j=0;j<i;j++)
        waysPerCombo *= numInSet;
    for (j=i;j<numDigits;j++)
        waysPerCombo *= numNotInSet;

    // Loop through each possible way to make an odd/even number of digits
    // up to numDigits (such as 1, 3, 5 in the example above).  For each
    // number of digits, there are (numDigits choose i) possible ways to
    // make a Dxxxx pattern, and waysPerCombo ways of filling that pattern
    // with digits.
    for (; i <= numDigits; i += 2) {
        ret += combinations(numDigits, i) * waysPerCombo;
        waysPerCombo /= numNotInSet * numNotInSet;
        waysPerCombo *= numInSet * numInSet;
    }
    return ret;
}

// This function computes the "digit xor sum" for the specified number and
// "bit" (either 1, 2, 4, or 8).  This bit is specified by the digit set that
// is passed in.  The number is given as an array of digits, with the least
// significant digit in digits[0].  For example, the number 573 would be
// passed in as the array { 3, 7, 5 }.
//
// digits   : An array of digits for a given number, with the least significant
//            digit in digits[0].  For example, if the number was 573, then the
//            digit array would be { 3, 7, 5 }.
// numDigits: The length of the digits array.
// digitSet : The set of digits we are solving for (e.g. ones, twos).
// numDigitSet: The length of the digitSet array.
// odd:       True if we are looking for an odd number of occurrences of the
//            specified bit.  False if we are looking for an even number.
static uint64_t computeXorSumForBit(int *digits, int numDigits,
                const uint8_t *digitSet, int numDigitSet, bool odd)
{
    int      i            = 0;
    int      leadingDigit = digits[numDigits-1];
    uint64_t sum          = 0;
    bool     findOdd      = false;

    // Special case for 0 digits.
    if (numDigits == 0)
        return odd ? 0 : 1;

    // For each digit leading up to the leading digit, we can use all of the
    // other digits.  For example, given the number 521, we can use all of
    // 0xx 1xx 2xx 3xx 4xx, but we can't use all of 5xx, only up to 521.
    //
    // If the digit we use is part of the set, then we need to flip the odd
    // variable because we are using 1 of that digit already.
    for (i=0; i<leadingDigit; i++) {
        findOdd = odd ^ isInSet(i, digitSet, numDigitSet);
        sum += computeCount(numDigits - 1, numDigitSet, findOdd);
    }

    // For the leading digit (5 in the 521 example), we are limited by the
    // other digits.  We can recurse into this same function, and solve for
    // one fewer digit.  For example, with 521, we solve for 21, and flip
    // the odd variable if 5 is part of the set.
    findOdd = odd ^ isInSet(leadingDigit, digitSet, numDigitSet);
    sum += computeXorSumForBit(digits, numDigits-1, digitSet, numDigitSet,
                findOdd);

    return sum;
}

// Computes the sum of "xor of digits" for all numbers from 1..n.  The xor of
// digits is the xor of each digit (base 10) in a number.  For example, if the
// number was 521, the xor of digits is 5 ^ 2 ^ 1 = 6.
//
// The method used here is to solve for each of the four bits (1, 2, 4, 8)
// separately.  For a particular bit such as 1, we count how many numbers
// between 1 and n have an odd number of digits containing the 1 bit (i.e.
// an odd number of these digits: 1,3,5,7,9).  These numbers will have an
// xor of digits value with 0x1 set.  All other numbers will have an xor
// of digits value with 0x1 clear.
static uint64_t computeXorSum(uint64_t n)
{
    uint64_t sum       = 0;
    int      numDigits = 0;
    int      digits[MAX_DIGITS] = {0};

    while (n > 0) {
        digits[numDigits++] = n % 10;
        n /= 10;
    }

    sum +=   computeXorSumForBit(digits, numDigits, ones,  sizeof(ones), true);
    sum += 2*computeXorSumForBit(digits, numDigits, twos,  sizeof(twos), true);
    sum += 4*computeXorSumForBit(digits, numDigits, fours, sizeof(fours), true);
    sum += 8*computeXorSumForBit(digits, numDigits, eights,sizeof(eights),true);

    return sum;
}

int main(void)
{
    uint64_t sum      = 0;
    int      numTests = 0;

    scanf("%d", &numTests);
    while (numTests--) {
        uint64_t n;

        scanf("%lld", &n);
        sum += computeXorSum(n);
    }
    printf("%lld\n", sum);
    return 0;
}

Old review (solves a different problem)

I don't know about \$O(1)\$ but it seems doable in \$O(\log n)\$.  Given a number \$n\$, you can compute how many times each digit appears in each decimal place.  For example, given the number 1000, I can tell you the digit 1 appears 1 time in the thousands digit, 100 times in the hundreds digit, 100 times in the tens digit, and 100 times in the ones digit, for a total of 301 times.

If you xor a digit an even number of times, you get 0.  If you xor a digit an odd number of times, you get the digit.  So in my example, you xor the digit 1 an odd number of times (301 times) and get 1.  Then you solve for digits 2..9 and xor all the digit results together.

Now the question is, how did I compute the count of the digit 1 in each decimal place?  I suggest you start with the ones place (the rightmost digit) and think about why the answer for any digit is always N/10 or N/10 + 1.  Then think about the tens place and think about why the answer is always between N/10 - (digit-1) and N/10 + (10-digit).  Once you figure out the pattern, you should be able to write a function like this:

int GetCount(int n, int digit, int decimalPlace);

Then your main computation would look like:

int ComputeDigitXor(int n)
{
    int totalXor = 0;
    int maxDecimalPlace = ComputeMaxDecimalPlace(n);
    for (int digit = 1; digit <= 9; digit++) {
        int count = 0;
        for (int place = 0; place <= maxDecimalPlace; place++) {
            count += GetCount(n, digit, place);
        }
        if (count & 1)
            totalXor ^= digit;
    }
    return totalXor;
}

I'll leave it up to the reader to figure out how to write GetCount() given the hints above.


Gather first letter from each string in a list of strings I'm learning Haskell on my own, and I'm wondering if this is idiomatic Haskell.

In particular: is it a common pattern to "wrap" the result in a list in order to make a function total? (that's what I ended up doing to ensure the function was total and no compiler warnings about not matching a pattern).

safeHead :: [a] -> Maybe a
safeHead []     = Nothing
safeHead (x:_)  = Just x

firstLetters :: [String] -> [Char]
firstLetters ss = concat $
                     map (\s -> case safeHead s of
                                Nothing -> []
                                Just c  -> [c]) ss

Well a few thoughts, of all String = [Char] so we could really give the signature

firstLetters :: [String] -> String

Whether you think this is better is a matter of taste. Second, you should take a look at

Data.Maybe.mapMaybe :: (a -> Maybe b) -> [a] -> [b]

It's exactly what you'd expect, it applies a function to Maybes and prunes out the Nothings. This simplifies the slightly ugly concat bit.

 firstLetters = mapMaybe safeHead

Much clearer :) In the interest of teaching you to fish, here's how I found the name of mapMaybe


Rosalind's 3rd problem in Scheme I have an imperative programming background and I've decided to study functional programming by applying it to problems found on sites such as Project Euler and Rosalind. My language of choice is Scheme (I use CHICKEN). As can be seen, I'm only starting out.

This is about the 3rd problem found on Rosalind, that goes like this:

  In DNA strings, symbols 'A' and 'T' are complements of each other, as
  are 'C' and 'G'.

  The reverse complement of a DNA string s is the string sc formed by
  reversing the symbols of s, then taking the complement of each symbol
  (e.g., the reverse complement of "GTCA" is "TGAC").

  Given: A DNA string s of length at most 1000 bp.

  Return: The reverse complement sc of s.

Here's my code, which solves the problem:

(define (complement-of char)
  (cond
    ((eq? char #\A)
     #\T)
    ((eq? char #\T)
     #\A)
    ((eq? char #\C)
     #\G)
    ((eq? char #\G)
     #\C)
    (else
      char)))

(define (apply-complements dna-string)
  (reverse-list->string
    (map complement-of
     (string->list dna-string))))

(define (solve-problem infile-name outfile-name)
  (let ((outfile-port (open-output-file outfile-name))
        (infile-port (open-input-file infile-name)))
    (begin
      (write-line
         (apply-complements (read-line infile-port)) outfile-port)
      (close-output-port outfile-port))))

Is there a nicer way to write complement-of? Perhaps something like a dictionary, or would that be too much work in this particular case?

Is solve-problem there appropriate from a style POV, or is this more appropriate?

(define (solve-problem infile-name outfile-name)
  (let ((outfile-port (open-output-file outfile-name))
        (infile-port (open-input-file infile-name)))
    (begin
      (write-line
         (apply-complements
           (read-line infile-port))
         outfile-port)
      (close-output-port outfile-port))))

Also, is there a way to avoid using begin and still make things look readable? The only idea that comes to mind is extracting a function and through recursion decide whether or not it writes to the file, or closes it. But that feels hardly intuitive.

Finally, I'd like to hear of any other issues my code has.

complement-of
The nicer way to write complement-of is with case instead of cond:
(case char
  ((#\A) #\T)
  ((#\T) #\A)
  ((#\C) #\G)
  ((#\G) #\C)
  (else char))

Function names generally shouldn't end in -of. (That's a way to pronounce function call, but it's not part of the name.) complement is enough. (Although some Schemes have a predefined function by that name, and it would be nice to not collide with it.)
Beware of eq? — it's not reliable on characters. (It's the equivalent of Java's == on Character). Use eqv? instead unless you know what you're doing. (case uses eqv? internally.)
apply-complements
You can simplify apply-complements by using one of Chicken's libraries: (use (srfi 13)) to get string-map, which is like map but saves you having to convert to a list. That plus string-reverse makes apply-complements easy.
...but I wouldn't call it apply-complements, because that sounds like a reference to the apply function. It returns the reverse complement of its argument, so that should be its name: reverse-complement.
If you want to make it faster, try the imperative approach: allocate a string with make-string, loop with do and string-set!. (It's useful to know how to write imperative code, even in functional languages.)
solve-problem
solve-problem has a very ordinary bug: you forgot to close the input port. :) Fortunately, there are functions that do this automatically so you don't have to remember: call-with-input-file and call-with-output-file. They're also safer, because they close the ports on nonlocal exits like exceptions.
The body of a let is an implicit begin, so the begin in solve-problem is not needed.
Formatting: when a function call is split across multiple lines, the arguments are usually aligned, like this:
(write-line (apply-complements (read-line infile-port))
            outfile-port)

solve-problem has a vague name. How about a description of what it does, such as reverse-complement-file? (Names are the best comments.)
...however, reverse-complement is only one of many operations you might want to apply to a file this way. I'd probably write it as a general-purpose transform-file that takes the operation as a parameter: (transform-file reverse-complement src dest). Or I'd write it as two separate operations for reading and writing, since they're often useful separately. (I'd also use read-string and write-string instead of read-line and write-line, so they'd work on files of more than one line.)


Sorting a String based on its words Is there a better, more efficient or nicer way to do this?

public static String sortString(String s){

    String[] strArray= s.split("\\s+");
    Arrays.sort(strArray);
    StringBuilder sb = new StringBuilder();
    for (int i=0; i<strArray.length; i++){
        sb.append(strArray[i]);
        sb.append(" ");
    }
    return sb.toString();
}

A couple of comments:

String[] strArray = s.split("\\s+"); will create a Pattern object each time this is called, which might hurt the performance. It would be better to reuse the same pattern. Declare it as a constant:

private static final Pattern PATTERN = Pattern.compile("\\s+");

and then you can use

String[] strArray = PATTERN.split(s);

Use a for-each loop instead of a loop using index. This is both easier to read and to maintain when you don't need the index. So instead of having

for (int i=0; i<strArray.length; i++){
    sb.append(strArray[i]);

you can have

for (String str : strArray){
    sb.append(str);

If you can use Java 8, you could have

public static String sortString(String s){
    return PATTERN.splitAsStream(s).sorted().collect(Collectors.joining(" "));
}

This splits the input String into a Stream<String> using splitAsStream. Then the stream is sorted and finally collected with a collector joining all elements together, separated by a space, using Collectors.joining(" "). As a side-note, this will not include the last white-space at the end of the joined Strings, unlike the code in your question.


What do you think of my regex for URL validation? I would like you to review my regex.  It's suppose to recognize common URLs like:

http://www.google.com
http://www.sub1.sub2.google.com
https://www.google.com
http://www.google.com/path1/path2
http://www.google.com/path1/path2/
http://www.google.com/path1/path2/a=b&c=d
http://www.google.com/path1/path2/a=b&c=d/

var url = new RegExp("^(?:https?:\/\/)?(?:[-a-z0-9]+\\.)+[-a-z0-9]+(?:(?:(?:\/[-=&?.#a-z0-9]+)+)\/?)?$");

Your regex has many false positives (matches without a valid URL being there), and many false negativs (URLs you don't recognize). You should read the appropriate RFCs which contain relevant parts of the grammar.

Here are some failure scenarios:

⊖ You don't match schemes other than HTTP or HTTPS. Consider FTP, SFTP, data URLs, and other widespread schemes like file:, mailto:, irc: or skype:. Consider also that the scheme is sometimes omitted, so that the same protocol as the current document is assumed. Test cases:

ftp://example.com/
data:text/html,<p>Hello World!
mailto:foo@example.com
//example.com/

Every scheme has a slightly different syntax, so you'll have to decide which ones you support. Note that matching a HTTP(S) URL is different from matching all URLs.
⊕ You match some domain names that are syntactically correct, but semantically only consist of a top level domain. You may or may not wish to rule these out:

http://co.uk/

See the Mozilla Public Suffix List for the only reliable way to do this.
⊖ Some host names don't have a TLD:

http://localhost/

⊕ You match loads of stuff that isn't ok, e.g.:

999.999.999.999/this-is-no-ip4-address
-.-

⊖ You don't have support for ports:

http://example.com:80/

⊖ You don't have support for IPv6 IPs.

http://::1/
http://[::1]:80/

⊖ You can't match any authority in the host part:

ftp://user:password@example.com/

⊖ You don't support URL-encoding:

http://example.com/some%20path

⊖ You don't support Unicode chars:

http://example.com/smørrebrød/greek-Λεττερς

Consider also that the host name may contain Unicode characters, but will have an equivalent punycode representation.
⊖ Coming to think of it, you actually forbid quite a lot of (special) characters, although they would be perfectly valid (the host part is more restricted).

http://example.com/stuff_(in parens)/

⊖ In a URL, the path is followed by a single query string, introduced by a ?. Note that the semicolon ; is the more modern, but equivalent alternative to the & separator. The + means a literal space here. The query string may only be followed by a fragment, which is separated by #. Other instances have to be encoded.

For an overview of URL syntax, start with this Wikipedia article. Also look at URI Schemes. These articles also link to relevant specs.


Bash: change git remote from SSH to HTTPS within a given dir The code takes a user input dir path, then looks for git remote url that is still using SSH, and the repo is belong to an fixed Organisation. Then it will convert these repos' git remote url from SSH to HTTPS.

Goal is to let users (on Linux or Mac) effortlessly convert all dir containing the Organisation's code to use HTTPS instead of SSH.

My senior programmer said problems in the code are so many that he has to sit down with me next week to talk through! Please help me to spot problems before embarrassment starts. Thank you

#!/usr/bin/env bash

# This script loops through all directories within the given path,
# and run a conversion script to convert Git CLI auth type.
GITHUB_USERNAME='Mock-Technology'

converter () {
  # This function will convert origin url from SSH style to HTTPS style, for the
  # git containing local directory

  if [[ -z $GITHUB_USERNAME ]]; then
    GITHUB_USERNAME='Mock-Technology'
  fi

  echo "-- converter starting..."
  cd "$1" || exit
  if [[ ! -f "config" ]]; then
    echo "This directory does not contain Git, exitting..."
    return
  else
    origin_str=$(cat "config")
  fi

  repo_url=$(echo "$origin_str" | sed -Ene's#.*(git@[^[:space:]]*).*#\1#p')
  if [[ -z "$repo_url" ]]; then
    echo "-- ERROR:  Could not identify Repo url."
    if [[ $origin_str == *"https"* ]]; then
      echo "   This repo is already using HTTPS instead of SSH."; else
      echo "   Valid remote URL was not found!"
    fi
    return
  fi
  echo "-- Confirmed: It is using SSH..."

  user_name=$(echo "$repo_url" | sed -Ene's#git@github.com:([^/]*)/(.*).git#\1#p')
  if [[ -z "$user_name" ]]; then
    echo "-- ERROR:  Could not identify User."
    return
  elif [ "$user_name" != "$GITHUB_USERNAME" ]; then
    echo "-- WARNING: The repo does not belong to '$GITHUB_USERNAME', ignoring..."
    return
  fi
  echo "-- User name extracted: $user_name"

  repo_name=$(echo "$repo_url" | sed -Ene's#git@github.com:([^/]*)/(.*).git#\2#p')
  if [[ -z "$repo_name" ]]; then
    echo "-- ERROR:  Could not identify Repo."
    return
  fi
  echo "-- Repo name extracted: $repo_name"

  new_url="https://github.com/$user_name/$repo_name.git"
  echo "-- Changing repo url from: "
  echo "  '$repo_url'"
  echo "      to "
  echo "  '$new_url'"
  echo ""

  change_cmd="git remote set-url origin $new_url"
  eval "$change_cmd"
  echo "-- Success!"

}

validate_path() {
  # this module validates input path against empty string and non-existing path
  if [[ -z $1 ]]; then
    echo "path is empty string, existting..."
    exit
  fi

  if [[ ! -d $1 ]]; then
    echo "path does not exist, exitting..."
    exit
  fi
  echo "-- path validation check passed..."
}

loop_subdirectories() {
  # visits subfolders and try to find any ".git" directory
  # It invokes converter func for each ".git" directory found
  while read -r dir; do
    converter "$dir"
  done <<< "$(find "$1" -name .git -type d)"

}

read -p "Enter path: " path
validate_path "$path"
loop_subdirectories "$path"

Extracting remote urls

Parsing the config file is error-prone. A much better way to extract the information about remotes is using Git commands. This will simplify your script a great deal.

Limitations

Be aware that the script as it is will not be able to convert remotes that were created using URL shorthands. For example, I have this in my global .gitconfig:

[url "git@github.com:"]
    insteadOf = "gh:"

This lets me clone a GitHub repo with git clone gh:user/repo instead of git clone git@github.com:user/repo. The config file will be written using the shorthand instead of the real URL, so your script will not be able to pick up the pattern.

You can easily solve this limitation by using Git commands to extract Git remote URLs.

String transformations

Instead of echo ... | sed ..., it's better to use here-strings:

sed ... <<< ...

But when the text transformations are simple enough, and Bash's parameter expansion can handle it, that's even better. For example, instead of this:

  user_name=$(echo "$repo_url" | sed -Ene's#git@github.com:([^/]*)/(.*).git#\1#p')

This would be better:

# chop off the beginning until the first :
user_name=${repo_url#*:}
# chop off everything after the first /
user_name=${user_name%%/*}

Exiting with error

There are multiple exit points in the validate_path function.
Without parameter, exit will use the exit code of the previous command, and if that was echo, it's likely to be success,
when in fact you probably want to exit with error instead.

Usability

Reading paths inside a script is not user-friendly,
because you cannot use tab-completion.
It would be better to let users pass paths as parameters to the script.


Extending react hook useState for use in event handlers I use React with material-ui.com and I love them both, but I'm tired with writing the boilerplate handlers like

onChange={(e: React.ChangeEvent<HTMLInputElement>) => setEmail(e.currentTarget.value)}

again and again. Especially when stopPropagation is needed (AFAICT it's always either needed or harmless), it gets pretty verbose.

So I tried to extend useState so that it provides everything I need, so I can write things like:

const [email, setEmail, emailAgg] = useStateEx('');
...
onChange={emailAgg.changeHandler}

or

const [showPassword, , showPasswordAgg] = useStateEx(false);
...
onClick={showPasswordAgg.toggleHandler}

and similar.

My approach follows. I'd like it to be reviewed in general, especially for better typing (I had to use @ts-ignore) and improvements.

I case you hate semicolons, please ignore them as my eslint is set up to require them. ;)

import { useState, Dispatch, SetStateAction } from 'react';

function booleanAgg(value: boolean, setter: (value: boolean) => void, makeSetter: (value: boolean) => ((event?: any) => void)) {
    return {
        value,
        setter,
        falseHandler: makeSetter(false),
        trueHandler: makeSetter(true),
        toggleHandler: makeSetter(!value),
    };
}

function stringAgg(value: string, setter: (value: string) => void) {
    return {
        value,
        setter,
        changeHandler: (e: React.ChangeEvent<HTMLInputElement|HTMLTextAreaElement>) => {
            e.stopPropagation();
            setter(e.currentTarget.value);
        }
    };
}

function numberAgg(value: number, setter: (value: number) => void) {
    return {
        value,
        setter,
        changeHandler: (e: React.ChangeEvent<HTMLInputElement|HTMLTextAreaElement>) => {
            e.stopPropagation();
            setter(+e.currentTarget.value);
        }
    };
}

/**
 * Works like useState and adds a third element containing boolean-specific handlers directly useable in buttons and checkboxes.
 */
export function useStateEx(initialState: boolean) : [boolean, Dispatch<SetStateAction<boolean>>, ReturnType<typeof booleanAgg>];

/**
 * Works like useState and adds a third element containing string-specific handlers directly useable in text fields.
 */
export function useStateEx(initialState: string) : [string, Dispatch<SetStateAction<string>>, ReturnType<typeof stringAgg>];

/**
 * Works like useState and adds a third element containing number-specific handlers directly useable in text fields.
 */
export function useStateEx(initialState: number) : [number, Dispatch<SetStateAction<number>>, ReturnType<typeof numberAgg>];

/**
 * Prohibits use with any type not handled in the above overloads.
 */
export function useStateEx(initialState: any) : never;

export function useStateEx<S extends boolean|string|number>(initialState: S) : unknown {
    const [value, setter] = useState(initialState);
    function makeSetter(value: S) {
        return function(e: any) {
            if (typeof e?.stopPropagation === 'function') e.stopPropagation();
            setter(value);
        };
    }
    if (typeof value === 'boolean') {
        // @ts-ignore
        return [value, setter, booleanAgg(value, setter, makeSetter)]
    } else if (typeof value === 'string') {
        // @ts-ignore
        return [value, setter, stringAgg(value, setter)];
    } else if (typeof value === 'number') {
        // @ts-ignore
        return [value, setter, numberAgg(value, setter)];
    } else {
        throw new Error('Only boolean, string and number is supported');
    }
}

I must admit that my experience with typescript and react.js is quite limited so my review will be limited to basic syntax points. This code seems straight-forward. It makes good use of arrow functions and destruction assignment. There are just a couple suggestions I will make below.

if (typeof e?.stopPropagation === 'function') e.stopPropagation();

It is best to include brackets around the block, even if it is all on one line:

if (typeof e?.stopPropagation === 'function') { e.stopPropagation(); }

Some believe such blocks should never be on one line. If you are going to do it on one line, you could use short-circuiting:

typeof e?.stopPropagation === 'function' && e.stopPropagation();

This block can be simplified somewhat:

if (typeof value === 'boolean') {
    // @ts-ignore
    return [value, setter, booleanAgg(value, setter, makeSetter)]
} else if (typeof value === 'string') {
    // @ts-ignore
    return [value, setter, stringAgg(value, setter)];
} else if (typeof value === 'number') {
    // @ts-ignore
    return [value, setter, numberAgg(value, setter)];
} else {
    throw new Error('Only boolean, string and number is supported');
}

The else keywords can be avoided because preceding blocks have return statements.'

if (typeof value === 'boolean') {
    // @ts-ignore
    return [value, setter, booleanAgg(value, setter, makeSetter)]
}
if (typeof value === 'string') {
    // @ts-ignore
    return [value, setter, stringAgg(value, setter)];
}
if (typeof value === 'number') {
    // @ts-ignore
    return [value, setter, numberAgg(value, setter)];
}
throw new Error('Only boolean, string and number is supported');


Resizing image but keeping aspect ratio I wrote some code that resizes an image to fit in a given box. I think that some parts of the code are not needed (for example if it's a box image). Is there a more efficient way to do this? Is there something wrong with my code (from the results it looks ok)? Is there a "shorter" version to do this?

Bitmap original, resizedImage;

try
{
    using (FileStream fs = new System.IO.FileStream(imageLabel.Text, System.IO.FileMode.Open))
    {
        original = new Bitmap(fs);
    }

    int rectHeight = BOXHEIGHT;
    int rectWidth = BOXWIDTH;

    //if the image is squared set it's height and width to the smallest of the desired dimensions (our box). In the current example rectHeight<rectWidth
    if (original.Height == original.Width)
    {
        resizedImage = new Bitmap(original, rectHeight, rectHeight);
    }
    else
    {
        //calculate aspect ratio
        float aspect = original.Width / (float)original.Height;
        int newWidth, newHeight;

        //calculate new dimensions based on aspect ratio
        newWidth = (int)(rectWidth * aspect);
        newHeight = (int)(newWidth / aspect);

        //if one of the two dimensions exceed the box dimensions
        if (newWidth > rectWidth || newHeight > rectHeight)
        {
            //depending on which of the two exceeds the box dimensions set it as the box dimension and calculate the other one based on the aspect ratio
            if (newWidth > newHeight)
            {
                newWidth = rectWidth;
                newHeight = (int)(newWidth / aspect);
            }
            else
            {
                newHeight = rectHeight;
                newWidth = (int)(newHeight * aspect);
            }
        }
        resizedImage = new Bitmap(original, newWidth, newHeight);
    }
}
catch (Exception ex)
{
    MessageBox.Show(ex.Message);
}

Your algorithm to calculate the final image dimensions could be improved by simplifying the logical process that you use to determine the final size.

Fundamentally, there are two dimensions you are interested in, the height, and width of the final image. There are two possible outcomes for the scaling, one where the scaling produces the target height, and the other where it produces the target width. The scaling that is used is the one which produces the smallest image. The size of the final image is determined by the scaling factor, and there are two factors:

float scaleHeight = (float)BOXHEIGHT / (float)original.Height;
float scaleWidth = (float)BOXWIDTH / (float)original.Width;

Now, we have two scaling ratios, which one produces the smaller image? The one that has the smallest scaling factor.

float scale = Math.Min(scaleHeight, scaleWidth);

Now you can simply do the adjustment:

resizedImage = new Bitmap(original,
         (int)(original.Width * scale), (int)(original.Height * scale));


Is this a good and simple example for Singleton Pattern This is a scenario where we keep a single instance(as in Singleton Pattern) to get the configuration of an application.

Below is the main class and i set values to the configuarion values here and call the methods of the other classes to check if they are getting the correct values.

public class A{
    public static void main(String []args){
        System.out.println("My Singleton example");
        System.out.println("Setting values to configs\n");
        configs conf = configs.getInstance();
        conf.setName("Home Page");
        conf.setUrl("www.google.com");

        B.showConfigValues();
        C.showConfigValues();

    }
}

Below is the singleton class.

public class configs{

    private static configs appConfig;
    private String name;
    private String url;

    private configs(){}

    public static configs getInstance(){
        if( appConfig == null){
            appConfig = new configs();
        }
        return appConfig;
    }

    public void setUrl(String url){
        this.url = url;
    }

    public void setName(String name){
        this.name = name;
    }

    public String getUrl(){
        return url;
    }

    public String getName(){
        return name;
    }
}

Below is class B and C. They access the configuration values in their methods.

public class B{

    public static void showConfigValues(){
        configs conf = configs.getInstance();
        System.out.println("Geting configs in Class B");
        System.out.println("url : "+conf.getUrl());
        System.out.println("name : "+conf.getName()+"\n");
    }
}

public class C{

    public static void showConfigValues(){
        configs conf = configs.getInstance();
        System.out.println("Geting configs in Class C");
        System.out.println("url : "+conf.getUrl());
        System.out.println("name : "+conf.getName()+"\n");
    }
}

Below is the output.

$ java A
My Singleton example
Setting values to configs

Geting configs in Class B
url : www.google.com
name : Home Page

Geting configs in Class C
url : www.google.com
name : Home Page

My question is, is this a good example to explain the singleton pattern?

For explaining the Singleton pattern the ABC naming is confusing. Just come up with some real name, e.g. Database, Logger etc. Something that might need access to a common config. Instead of System.out the config, you might want to add some mock code to init the database connection or get the logging directory.

Please keep in mind that Java classes usually are starting with a capital letter.

The static instance variable is usually called only instance.

In which context do you want to explain the pattern. Maybe we find a more suitable example?

Check http://www.oodesign.com/singleton-pattern.html for a nice explanation and some examples and drawbacks.

Please be aware that a Singleton might seem to help you sometimes in production code, but it definitely makes your life harder in testing your code. See "Dependency Injection" for an alternate approach.


ASP.NET Product Listing I am working on a e-shop in ASP.NET MVC, and I just made the listing of product the way I figured out this night. Is it correct? Or should I make it differently? How?

So this is my Product controller, used for displaying the products from Db.

using WebShop.Models;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.Mvc;

namespace WebShop.Controllers
{
    public class ProductController : Controller
    {
        private WebShopEntities db = new WebShopEntities();

        public ActionResult Category(int? id)
        {
            int pid = id == null ? 1 : (int)id;  //if requested id is null, set is as 1
            if (db.Categories.Find(pid) == null)  //if category does not exist, return Error
                return View("Error");
            var model = db.Categories.Find(pid).Products.ToList(); //return list of projects in requested category
            return View(model);
        }
    }
}

This is the Category View.

@using WebShop.Models
    @{
        ViewBag.Title = "Category";
        Layout = "~/Views/Shared/_Layout.cshtml";
    }

            @Html.Partial("Partial/_PartialCategoryMenu")  //display partial view for category menu

            @foreach(Product p in Model)
            {
                @Html.Partial("Partial/_PartialProduct", p)  //display partial view
            }

These are partial Views, which are self-explanatory I hope.
Category partial view

@using WebShop.Models

@using (var db = new WebShopEntities()) {
foreach (var category in db.Categories)
{
    @category.name
}
}

Product partial view

@Model.name

@Model.price €

@Model.description

To Cart Details

I would definitely remove the database access from the Category partial view and move it to the Controller (you can add it to the view model or have a separate action for returning them which is even more clean).

Views are supposed to be dumb layouts and not perform any business logic. Removing the database context shouldn't affect the compilation of the views.

Small refactoring suggestion for the Controller:

    public ActionResult Category(int? id)
    {
        var pid = id ?? 1;  //if requested id is null, set is as 1

        var category = db.Categories.Find(pid);

        if (category == null)  //if category does not exist, return Error
            return View("Error");

        var model = category.Products.ToList(); //return list of projects in requested category

        return View(model);
    }

Don't call find twice will save an extra database access which is important and code style wise I would use the ?? operator as you see above.

What the View("Error") is doing? Do you have an error view or is it just raising an exception? In both cases I would be more specific.

I would also make the view strongly typed as it will prevent errors and give you IntelliSense.


Take previous element from array if condition on current element matches Any ideas to simplify this beauty; I would prefer a LinQ expression if possible:

private object[] array;
public abstract bool Condition(object o);
//...
private object FindStuff()
{
    for (int i = 0; i < array.Length; i++)
    {
        if (Condition(array[i]))
        {
            return i == 0 ? null : array[i-1];
        }
    }
    throw new ItemNotFoundException();
}
//...

This code would benefit from a previous variable...... and it would also benefit from being real code, not this hypothetical example..... This code is also really short, so it's hard to simplify more.

Still, using a foreach is better than the indexed iterator, and the logic is more obvious with named variables, rather than indexes... so:

private object[] array;
public abstract bool Condition(object o);
//...
private object FindStuff()
{
    object previous = null;
    foreach (object current in array)
    {
        if (Condition(current))
        {
            return previous;
        }
        previous = current;
    }
    throw new ItemNotFoundException();
}


Creating and displaying a checkers board I am learning clojure this week end and just starting to develop a simple checkers engine. I currently coded some basic functions to create the board and display it. I would love to get some feedback !

(ns sandbox.checkers)

(def standard-size 10)

(defn empty-board [size]
  (vec (replicate size (vec (replicate size :empty)))))

(defn str-piece [piece]
  (cond
    (= piece :empty) " "
    (= piece :white_man) "O"
    (= piece :black_man) "0"))

(defn str-board [board]
  (str "\n"
    (clojure.string/join
      "\n"
      (map
        #(clojure.string/join " | " (map str-piece %))
        board))))

(defn print-board [board]
  (println (str-board board)))

(defn fill-white [size]
  (apply comp
  (map
    (fn [pos]
      (fn [board] (assoc-in board pos :white_man)))
      (for [x (range size)
            y (range (inc (/ size 2)) size)
            :when (or (and (even? x) (even? y))
                      (and (odd? x) (odd? y)))]
        [y x]))))

(defn fill-black [size]
  (apply comp
    (map
      (fn [pos]
        (fn [board] (assoc-in board pos :black_man)))
      (for [x (range size)
            y (range 0 (dec (/ size 2)))
            :when (or (and (even? x) (even? y))
                      (and (odd? x) (odd? y)))]
        [y x]))))

(defn starting-board [size]
  ((fill-black size) ((fill-white size) (empty-board size))))

(print-board (starting-board standard-size))

A few small points

replicate is deprecated. Use repeat instead.
Use - instead of _ in names.
The cond in str-piece compares piece against a number of
constants. Use case instead.
Simplify the :when conditions to (= (even? x) (even? y))

This gives us

(ns sandbox.checkers)

(def standard-size 10)

(defn empty-board [size]
  (vec (repeat size (vec (repeat size :empty)))))

(defn str-piece [piece]
  (case piece
    :empty " "
    :white-man "O"
    :black-man "0"))

(defn str-board [board]
  (str "\n"
    (clojure.string/join
      "\n"
      (map
        #(clojure.string/join " | " (map str-piece %))
        board))))

(defn print-board [board]
  (println (str-board board)))

(defn fill-white [size]
  (apply comp
         (map
           (fn [pos]
             (fn [board] (assoc-in board pos :white-man)))
           (for [x (range size)
                 y (range (inc (/ size 2)) size)
                 :when (= (even? x) (even? y))]
             [y x]))))

(defn fill-black [size]
  (apply comp
    (map
      (fn [pos]
        (fn [board] (assoc-in board pos :black-man)))
      (for [x (range size)
            y (range 0 (dec (/ size 2)))
            :when (= (even? x) (even? y))]
        [y x]))))

(defn starting-board [size]
  ((fill-black size) ((fill-white size) (empty-board size))))

(print-board (starting-board standard-size))

Edited subsequently:

There are simpler ways to generate the lists of squares for fill-white and fill-black.

For a given row y, the required xs are (range (mod y 2) size 2). The functions become

(defn fill-white [size]
  (apply comp
         (map
           (fn [pos] (fn [board] (assoc-in board pos :white-man)))
           (for [y (range (inc (/ size 2)) size)
                 x (range (mod y 2) size 2)]
             [y x]))))

(defn fill-black [size]
  (apply comp
         (map
           (fn [pos] (fn [board] (assoc-in board pos :black-man)))
           (for [y (range 0 (dec (/ size 2)))
                 x (range (mod y 2) size 2)]
             [y x]))))

A thorough reworking

The fill... functions construct and apply a sequence of functions, one function for each change to the empty board.

It is simpler to describe the repetitive pattern using functions cycle and take, with vec to convert to vectors:

(defn starting-board [size]
  (let [block (fn [rows piece]
                (let [source (cycle [piece :empty])
                      row-pair (map (comp vec (partial take size))
                                    [source (rest source)])]
                  (take rows (cycle row-pair))))
        block-size (dec (quot size 2))]
    (vec (concat (block block-size :black-man)
                 (block 2 :empty)
                 (block block-size :white-man)))))

Notice that the same row vectors are used again and again, not copies. This works in Clojure because vectors (and other native data structures) are immutable.


Is this the best way to collapse a column width? Requirements:
When the page (or in the case of the example Frame) expands, the only column to expand with the table should be 'unfrozen' columns (without the 'freeze' class).

here is the live demo The jQuery is there to show dynamic content, it is not the code under review

What I really don't like is that is seems to hacky to just set the width of the column to 1px... I can forsee this causing issues with older/junkier browsers.

HTML

            id
            name
            description
            something else
            actions

CSS

.data-table {
    width: 100%;
    font-size: 90%;
    background-color: #fff;
    border: 1px solid #666;
}
.data-table th {
    white-space: nowrap;
    text-align: center;
}
.data-table td {
    font-size: 90%;
    color: #222;
}
.data-table td:last-child {
    white-space: nowrap;
}
.data-table td, .data-table th {
    padding: 6px;
    border-right: 1px solid #999;
}
.data-table td:last-child, .data-table th:last-child {
    border: none;
}
.data-table tbody tr {
    border: 1px solid #666;
}
.data-table tbody tr:nth-child(odd) td {
    background-color: #efefef;
}
.freeze {
    width: 1px;
}

This works in at least IE8, Chrome 16, and FF8

IMHO it's completely fine to set the width like this. Table cell widths have always be defined to be a "minimum width".

As long the table doesn't have table-layout: fixed or the cells don't have overflow set to anything else than visible, I don't believe any browser (old or new) would have any problems width this.

The only nitpick I would have, it that the size of 1px feels wrong to me, mostly because this forces the column to wrap as much as possible and depending on the content this may not look very good. I would suggest 1em (purely because it "feels" better :-) and maybe a larger minimal width such as 10em would be suitable for some columns.


Reverse a String in Java Here is the code for the CString class that I created.

public class CString {

    public String reverse(String s) {
        char[] array = new char[s.length()];
        array = s.toCharArray();
        for(int i=0; i<array.length/2; i++) {
            char tmp = array[i];
            array[i] = array[array.length-1-i];
            array[array.length-1-i] = tmp;
        }
        return charArrayToString(array);
    }

    public String charArrayToString(char[] array) {
        StringBuffer sb = new StringBuffer();
        sb.append(array);
        return sb.toString();
    }
}

Is there any bug in the code? Are there more efficient ways to reverse a string?

Reversing a string, accurately and efficiently.

You are doing it accurately, yes. But, not very efficiently (although there are worse ways to do things).

NOTE:: Palacsint is right that you are not handling null input and surrogate-pairs in the input data. Your solution is not completely accurate. Consider this answer a discussion of the efficiency only.... though I have also updated this answer to include a solution which deals with surrogate pairs efficiently

To understand the efficiency side of things, you have to understand Java String internals.

Strings are immutable, cannot be changed.
Java is relatively slow at allocating/cleaning memory, try to avoid it.
Java can 'inline' method calls and make methods really fast.

So, the best way to make Java efficient is to create/use as little memory as possible, and still end up with a new String (instead of changing an existing string).

Your code has 1 string already, you are creating another string. You cannot avoid that.

What you can avoid though is what happens in between.

Your 'In Between'

What you are doing in between is a bit messy, and wasteful:

    char[] array = new char[s.length()];
    array = s.toCharArray();

You create a new array, then you throw it away, and replace it with the s.toCharArray()... why?

Could be just:

    char[] array = s.toCharArray();

Also, you have a loop that swaps the chars... this is implemented like:

for(int i=0; i<array.length/2; i++) {

This could be faster if you did it backwards ( could be - depending on which Java you use).

for (int i = array.length / 2; i >= 0; i--) {

This way it only has to do the array.length / 2 one time (though, as I say, some Java implementations will perhaps compile it to work out for you).

Finally, the charArrayToString() is serious overkill....

Converting to a StringBuilder then to a String is a waste of time/resources...

return charArrayToString(array);

can be

return new String(array);

EDIT: Also, as has been mentioned by Richard Miskin... and I assumed you were already using a StringBuilder .... StringBuilder is much more efficient in a single-thread situation than StringBuffer. Unless you have good reason, you always should use StringBuilder.

Efficient in-between ....

public String reverse(String s) {
    char[] array = s.toCharArray();
    char tmp;
    for(int i = (array.length - 1) / 2; i >= 0; i--) {
        tmp = array[i];
        array[i] = array[array.length-1-i];
        array[array.length-1-i] = tmp;
    }
    return new String(array);
}

Edit: a different way:

Here are some performance numbers...

String Reverse                           => 4473870 (hot 19.74881ms)
String Reverse Surrogate Aware           => 4473870 (hot 22.73488ms)
String Reverse B                         => 4473870 (hot 25.16192ms)
String Reverse StringBuilder             => 4473870 (hot 31.60709ms)
String Reverse StringBuilder NoNullCheck => 4473870 (hot 31.72952ms)
String Reverse Orig                      => 4473870 (hot 36.83827ms)

For each of those 'hot' runs, I am reversing the order of 479829 words (linux.words) (and there are 4473870 characters in the data excluding newlines).

the code I suggested above as the 'efficient in-between' does it in 20 milliseconds
based on the discussion about null and Surrogate Pairs, the following code does this 'right', and runs in 23 milliseconds:

public String reverse(final String s) {
    if (s == null) {
        return null;
    }
    final char[] array = s.toCharArray();
    char tmp;
    for(int i=array.length/2; i >= 0; i--) {
        tmp = array[i];
        array[i] = array[array.length-1-i];
        array[array.length-1-i] = tmp;
    }
    //surrogate pairs will have been swapped.
    //identify, and un-swap them.
    for (int i = 1; i < array.length; i++) {
        if (Character.isHighSurrogate(array[i]) && Character.isLowSurrogate(array[i - 1])) {
            tmp = array[i];
            array[i] = array[i - 1];
            array[i - 1] = tmp;
        }
    }
    return new String(array);
}

The following code does it in 25 milliseconds

public String reverse(String s) {
    char[] array = new char[s.length()];
    for(int i=array.length - 1, j = 0; i >= 0; i--, j++) {
        array[i] = s.charAt(j);
    }
    return new String(array);
}

@palacsint's recommendation does it in 31 milliseconds:

public static String reverse(final String str) {
    if (str == null) {
        return null;
    }
    return new StringBuilder(str).reverse().toString();
}

@palacsint's recommendation (without the null-check) does it in 31 milliseconds:

public static String reverse(final String str) {
    return new StringBuilder(str).reverse().toString();
}

Your code does it in 37milliseconds.

If you look at the code, my code creates three objects (char[] and new String() (which creates char[] as well))

The s.charAt() code also creates three objects, but has a lot of calls to String.charAt().

The @palacsint recommendation creates 4 objects (StringBuffer, StringBuffer's internal char[], String, and String's internal char[]);

That is true with and without the null-check.

Your code creates 5 objects (6 if you count the first array which is likely to be compiled out...)  : (char[] array, new StringBuffer, StringBuffer's char[], String, and String's char[])

My guess is that there is a close correlation between our times simply because it takes 5ms to create 480,000 objects on the heap, plus some overhead of actual work.


Checking for a date and compare 2 different files From snippets around the web, I was able to put together this .bat file to check for a date and compare 2 different files.

This works below but I want to clean it up so it can be much lighter and faster and MUCH less code.

As you can see, it uses the same code to check date with hour and number. It is basically checking to see if the .xml file is older than 15 minutes and then will download a new version if over 15 minutes.

@echo off
cls
setlocal EnableDelayedExpansion

SET execTime=15

REM lets get XML date
SET xml=my.xml
FOR %%i IN (%xml%) DO SET fileDate=%%~ti
SET timeString=%fileDate%

:: Get the "AM PM" string to check the HOUR interval
FOR /F "tokens=1-3 delims= " %%A IN ('echo %timeString%') DO SET AM_PM=%%C

:: first  split so can delele the date
FOR /F "tokens=1-5 delims= " %%A IN ('echo %timeString%') DO SET theDate=%%A& SET HOUR=%%B

:: fix up the date to only numeric
set fixDate=%theDate%
set xmlDate=%fixDate: =%
set xmlDate=%fixDate:/=%

::split by ":" using "HOUR" set above
FOR /F "tokens=1-5 delims=:" %%A IN ('echo %HOUR%') DO SET H_NUM=%%A& SET M_NUM=%%B

:: Set HOUR intervals to "24 hour" format
IF %AM_PM%==PM (
IF %H_NUM%==01 (SET H_NUM=13)
IF %H_NUM%==02 (SET H_NUM=14)
IF %H_NUM%==03 (SET H_NUM=15)
IF %H_NUM%==04 (SET H_NUM=16)
IF %H_NUM%==05 (SET H_NUM=17)
IF %H_NUM%==06 (SET H_NUM=18)
IF %H_NUM%==07 (SET H_NUM=19)
IF %H_NUM%==08 (SET H_NUM=20)
IF %H_NUM%==09 (SET H_NUM=21)
IF %H_NUM%==10 (SET H_NUM=22)
IF %H_NUM%==11 (SET H_NUM=23)
) ELSE (
IF %H_NUM%==12 (SET H_NUM=12)
IF %H_NUM%==1 (SET H_NUM=01)
IF %H_NUM%==2 (SET H_NUM=02)
IF %H_NUM%==3 (SET H_NUM=03)
IF %H_NUM%==4 (SET H_NUM=04)
IF %H_NUM%==5 (SET H_NUM=05)
IF %H_NUM%==6 (SET H_NUM=06)
IF %H_NUM%==7 (SET H_NUM=07)
IF %H_NUM%==8 (SET H_NUM=08)
IF %H_NUM%==9 (SET H_NUM=09)
)

SET TIMESTAMP=%H_NUM%%M_NUM%
SET timeXmlCreated=%TIMESTAMP%

REM <<<-------------   NOW lets create bat to check time to compare time now  ----------------->>>

echo/|set /p =%date% >time.bat
time /t >>time.bat

SET mktime=time.bat
FOR %%i IN (%mktime%) DO SET timeFile=%%~ti
SET timeString=%timeFile%

:: Get the "AM PM" string to check the HOUR interval
FOR /F "tokens=1-3 delims= " %%A IN ('echo %timeString%') DO SET AM_PM=%%C

:: first  split so can delele the date
FOR /F "tokens=1-5 delims= " %%A IN ('echo %timeString%') DO SET theDate=%%A& SET HOUR=%%B

:: fix up the date to only numeric
set fixDate=%theDate%
set todaysDate=%fixDate: =%
set todaysDate=%fixDate:/=%

::split by ":" using "HOUR" set above
FOR /F "tokens=1-5 delims=:" %%A IN ('echo %HOUR%') DO SET H_NUM=%%A& SET M_NUM=%%B

:: Set HOUR intervals to "24 hour" format
IF %AM_PM%==PM (
IF %H_NUM%==01 (SET H_NUM=13)
IF %H_NUM%==02 (SET H_NUM=14)
IF %H_NUM%==03 (SET H_NUM=15)
IF %H_NUM%==04 (SET H_NUM=16)
IF %H_NUM%==05 (SET H_NUM=17)
IF %H_NUM%==06 (SET H_NUM=18)
IF %H_NUM%==07 (SET H_NUM=19)
IF %H_NUM%==08 (SET H_NUM=20)
IF %H_NUM%==09 (SET H_NUM=21)
IF %H_NUM%==10 (SET H_NUM=22)
IF %H_NUM%==11 (SET H_NUM=23)
) ELSE (
IF %H_NUM%==12 (SET H_NUM=12)
IF %H_NUM%==1 (SET H_NUM=01)
IF %H_NUM%==2 (SET H_NUM=02)
IF %H_NUM%==3 (SET H_NUM=03)
IF %H_NUM%==4 (SET H_NUM=04)
IF %H_NUM%==5 (SET H_NUM=05)
IF %H_NUM%==6 (SET H_NUM=06)
IF %H_NUM%==7 (SET H_NUM=07)
IF %H_NUM%==8 (SET H_NUM=08)
IF %H_NUM%==9 (SET H_NUM=09)
)

SET TIMESTAMP=%H_NUM%%M_NUM%
SET timeNow=%TIMESTAMP%
GOTO :dateDiff

:dateDiff
:: lets check dates now
SET /a dateDiff=%xmlDate%-%todaysDate%
set dateDiff=%dateDiff:-=%
if %dateDiff% GTR 0 goto timeDiff

:: do something here if more than zero

:timeDiff
::get time difference
::  must add a "1" to fix errors if starts with a "zero"
SET /a timeDiff=1%timeXmlCreated%-1%timeNow%
set timeDiff=%timeDiff:-=%
if %timeDiff% GTR %execTime% goto greaterThan

:lessThan
echo less than %execTime% min just leave alone
GOTO cleanUp

:greaterThan
echo more than %execTime% min lets delete
GOTO cleanUp

:cleanUp
Del  time.bat

endlocal
PAUSE > NUL

General issues:

1) You should learn how to create and call batch functions so you have re-usable code. See the batch function tutorial at DosTips.com. (Note that the name of the site is misleading. It is primarily a Windows batch site, not DOS.)

2) You put the current date and time in the temporary "time.bat" file, but you never use that data. Instead you use the last modifed timestamp of the newly created file to get the current timestamp. You could have used the dynamic %TIME% and %DATE% "variables" to get the current timestamp without using any temp file. However, there is a much better way to get the current timestamp.

3) Your temporary file has a .bat extension, so it could be executed (with failure of course, since it does not contain valid batch commands). If you create a temp file to house data, then it should have a benign extension. Often times .tmp is used. But again, you really don't need a temp file at all.

4) There is no guarantee that the user has write access to the current directory. When creating temp files, it is best to place them in the "%temp%" folder. This variable is preset for each user to point to a safe place to write temporary data.

5) You use the "time.bat" literal repeatedly, but then you define mktime and use %mktime% in your FOR statement. Why? You should be consistent. Either use the literal everywhere, or better yet, define a variable early on containing the full path to your temp file, and then use that variable consistently when referencing the temp file.

6) You create an excess of variable names. For example, you define TIMESTAMP, only to transfer the value to TIMENOW (or timeXmlCreated), and you never use the value of TIMESTAMP again. You should simply create TIMENOW and timeXmlCreated directly.

7) All of your time / date manipulation is highly dependent on the locale settings of the machine. Your code will break if it is moved to another machine that uses different formats for date and/or time. You should strive for code that is not dependent on locale settings. WMIC DATAFILE, and WMIC OS can be used to get locale indpendent timestamp information.

WMIC timestamps have the format of YYYYMMDDhhmmss.ffffffSzzz

 where:

   YYYY   = gregorian year
   MM     = month
   DD     = day
   hh     = hour in 24 hour format
   mm     = minutes
   ss     = seconds
   ffffff = fractional seconds (microseconds)
   S      = timezone sign: + or -
   zzz    = timezone: minutes difference from UTC

You can use the following to get the current timestamp:

set "localTime="
for /f "skip=1" %%A in (
  'wmic os get localdatetime'
) do if not defined localTime set "localTime=%%A"

You can use the following to get the last modified timestamp of a file. Note that WMIC requires that backslashes be escaped as \\ within the path.

set "fileTime="
for /f "skip=1" %%A in (
  'wmic datafile where "name='c:\\fullPath\\yourFile.ext'" get lastModified'
) do if not defined fileTime set "fileTime=%%A"

8) Ideally, you want to convert the date and time (timestamp) into an integral number so that you can do simple interval arithmetic. One common format is something called unix time - the number of seconds since midnight Coordinated Universal Time (UTC), January 1, 1970 (disregarding leap seconds). Timestamps before January 1, 1970 have negative unix times. I have written a batch function called :UnixTime to compute the unix time for a given timestamp, or for the current time. You can then simply subtract the file unix time from the current unix time, and look for a value > 900 (15 min * 60 sec/min)

Some important notes about the :UnixTime code.

SET /A can access the value of variables directly without expansion.
SET /A can make multiple computations and assignments with one call.
Batch treats numbers prefixed with 0 as octal. So a value of 09 would be an error, since 9 is not an octal digit. That is why the formula inserts prefixes before the substrings, and then uses modulo arithmetic to derive the correct value.

9) Your code to convert the hour into zero padded 24 hour format is flawed. It gives a value of 12 for 12 AM when it should be 00. It is also needlessly wordy.

Here is one concise way to do it:

if %H_NUM% equ 12 set "H_NUM=00"
if %AM_PM% equ PM set /a H_NUM=1%H_NUM%+12-100
if %H_NUM% lss 10 set "H_NUM=0%H_NUM%"

And here is an even more concise way:

set /a "AM=0, PM=12, H_NUM=(1%H_NUM%-100)%%12+%AM_PM%"
if %H_NUM% lss 10 set "H_NUM=0%H_NUM%"

Here is how I would write the code:

@echo off
setlocal

set "file=my.xml"
set "execTime=15"

:: get full path to file
for %%F in ("%file%") do set "file=%%~fF"

:: escape the backslashes
set "file=%file:\=\\%"

:: get the last modified timestamp for the file
set "fileTime="
for /f "skip=1" %%A in (
  'wmic datafile where "name='%file%'" get lastModified'
) do if not defined fileTime set "fileTime=%%A"

:: convert file timestamp to unix time format
call :UnixTime fileTime %fileTime%

:: get current timestamp in unix time format
call :Unixtime currentTime

:: compute interval in seconds, and convert execTime into seconds
set /a "diff=currentTime-fileTime, execSec=execTime*60"

:: what should we do?
if %diff% gtr %execSec% (
  echo greater than %execTime% min: let's delete
) else (
  echo less than or equal to %execTime% min: just leave alone
)

:: Must exit so we don't fall into the function below
exit /b

:UnixTime  [ReturnVar]  [TimeStamp]
::
:: Computes the Unix time from the current local time as reported by the
:: operating system. The Unix time is the number of seconds that have elapsed
:: since midnight Coordinated Universal Time (UTC), January 1, 1970, not
:: counting leap seconds.
::
:: The result is returned in variable ReturnVar,
:: or the result is echoed if ReturnVar is not specified
::
:: If the TimeStamp is provided in the 2nd parameter, then the Unix time for
:: the TimeStamp is computed, rather then for the current time.
::
:: The TimeStamp must have the same format as used by WMIC:
::
::   YYYYMMDDhhmmss.ffffffSzzz
::
:: where:
::
::   YYYY   = gregorian year
::   MM     = month
::   DD     = day
::   hh     = hour in 24 hour format
::   mm     = minute
::   ss     = seconds
::   ffffff = fractional seconds (microseconds)
::   S      = timezone sign: + or -
::   zzz    = timezone: minutes difference from GMT
::
:: Each component must be zero prefixed as needed to maintain the proper width.
::
:: The ReturnVar parameter must be provided in order to use the TimeStamp.
:: A ReturnVar of "" will function the same as no ReturnVar. This enables the
:: specification of a TimeStamp without an actual ReturnVar.
::
:: Note this function will fail with dates >= 2038-01-19 because the
:: unix time exceeds the maximum value of a signed 32 bit integer.
::
setlocal
set "ts=%~2"
if not defined ts for /f "skip=1 delims=" %%A in ('wmic os get localdatetime') do if not defined ts set "ts=%%A"
set /a "yy=10000%ts:~0,4% %% 10000, mm=100%ts:~4,2% %% 100, dd=100%ts:~6,2% %% 100"
set /a "dd=dd-2472663+1461*(yy+4800+(mm-14)/12)/4+367*(mm-2-(mm-14)/12*12)/12-3*((yy+4900+(mm-14)/12)/100)/4"
set /a ss=(((1%ts:~8,2%*60)+1%ts:~10,2%)*60)+1%ts:~12,2%-366100-%ts:~21,1%((1%ts:~22,3%*60)-60000)
set /a ss+=dd*86400
endlocal & if "%~1" neq "" (set %~1=%ss%) else echo %ss%
exit /b


A simple error messaging and logging system via macro(s) in C++ I'm just trying to build a quick and crude error messaging system. It currently looks like this:
#include <exception>
#include <fstream>
#include <iostream>
#include <string>
#include <sstream>
#include <map>

enum MsgTy {
    OK = 0,
    WARNING,
    ERROR,
    CRITICAL,
};

class FileWriter {
    std::string filename_;
    std::ostringstream msg_;
public:
    FileWriter(const std::string& filename, std::ostringstream& msg)
      : filename_{ filename } {
        operator()(msg);
    }

    void operator()(std::ostringstream& msg) {
        std::ofstream out("log.txt", std::ios::app);
        out << msg.str();
    }
};

static std::map<MsgTy, std::string> msg_id{
    {MsgTy::OK, {"OK: "}},
    {MsgTy::WARNING, {"WARNING: "}},
    {MsgTy::ERROR, {"ERROR: "}},
    {MsgTy::CRITICAL, {"CRITICAL: "}}
};

#define messaging(MsgTy, msg, log2file) do { \
    std::ostringstream strm; \
    if ((MsgTy) == OK) { \
        strm << msg_id[(MsgTy)] << (msg) << '\n'; \
        std::cout << strm.str(); \
        if((log2file) == true) \
            FileWriter fw("log.txt", strm); \
    } \
    if ((MsgTy) == WARNING) { \
        strm << msg_id[(MsgTy)] << (msg) << '\n'; \
        std::cout << strm.str(); \
        if((log2file) == true) \
            FileWriter fw("log.txt", strm);\
    } \
    if ((MsgTy) == ERROR) { \
        strm << msg_id[(MsgTy)] << (msg) << '\n'; \
        std::cerr << strm.str(); \
        if((log2file) == true) \
            FileWriter fw("log.txt", strm); \
        throw strm.str(); \
    } \
    if ((MsgTy) == CRITICAL) { \
        strm << msg_id[(MsgTy)] << (msg) << '\n'; \
        std::cerr << strm.str(); \
        if((log2file) == true) \
            FileWriter fw("log.txt", strm); \
        throw strm.str(); \
    } \
} while(0)

int main() {
    try {
        messaging(MsgTy::OK, "Everything is good!", true);
        messaging(MsgTy::WARNING, "Something isn't quite right!", false);
        messaging(MsgTy::ERROR, "Something went wrong!", true);
        messaging(MsgTy::CRITICAL, "Something horribly went wrong!", true);
    }
    catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;
}

I'm using a combination of several techniques... I'm using a class as a functor object for writing to basic text files, currently, it will only append to the file if it already exists or tries to create one. The file writer will only be invoked if the condition flag is true within the messaging system.
I'm using an enumeration and a static map to hold the basic strings for the different types of errors, warnings, or messages that my application or library might use. Then I'm using macro expansion to act as a function. Specific Error Message Types will also throw an exception and halt the program, while others, will just log to the console and let execution continue.
Yes I know I could have just written a function, class, functor, etc. and I know that macros can be tricky to get correct and are harder to debug, but this was also an exercise to refresh my skills at writing good solid macros.
What I would like to know:

Are there any apparent issues with how I implemented the macro?
Can there be any improvements made to it?
Is the intent clear on its usage?
I'm also interested in any other feedback.

Note -This is not production code: it's for a personal project, however, I would still like it to be critiqued as if it were to be production code!-

You’ve already noted that this could be done better without macros, so I won’t belabour the point. I will note, though, that your goal—“to refresh [your] skills at writing good solid macros”—makes about as much sense as refreshing your skills at writing code on punch cards. You are exercising an archaic practice that is dying out, and is unwelcome in any modern project.
enum MsgTy {
    OK = 0,
    WARNING,
    ERROR,
    CRITICAL,
};

In modern C++, you should use a strong enum—an enum class. That way your enumerators won’t pollute the namespace.
Speaking of polluting the namespace, the almost universal convention in C++ is that all-caps identifiers are used for preprocessor defines. By using them in this case, you run the risk of someone else’s macro definitions fuggering up your enum. And given that having a macro named something like ERROR is highly likely in large enough projects, you’re really cruising for a bruising here. (Actually, POSIX reserves everything starting with E followed by a digit or uppercase letter… so you’re REALLY asking for trouble with that in particular.)
I’m also not keen on the name MsgTy. Seems a little ugly and obtuse. I get that you want it to be short but… this seems a bit much.
class FileWriter {
    std::string filename_;
    std::ostringstream msg_;
public:
    FileWriter(const std::string& filename, std::ostringstream& msg)
      : filename_{ filename } {
        operator()(msg);
    }

    void operator()(std::ostringstream& msg) {
        std::ofstream out("log.txt", std::ios::app);
        out << msg.str();
    }
};

Oi, this class is….
First off… what is the point of the data members? You don’t use either of them.
Second… what’s the point of the function call operator? You could just as well do all the work in the constructor. You never use the function call operator anywhere else.
Third… what’s the point of taking the argument as a string stream when you just go ahead and re-format it through a file stream? You’re double-formatting it.
This entire class could boil down to:
struct FileWriter
{
    FileWriter(std::string_view filename, std::string_view msg)
    {
        auto out = std::ofstream{filename, std::ios_base::app};
        out << msg;
    }
};

But even then, I’m not sure this is a great idea, because you’re reopening the file every time you’re writing a new log line, then closing it after. That doesn’t seem like a great idea, efficiency-wise.
A better idea would be to open the file once, and keep it open. Then just syncronize your writes (assuming you care about concurrency, which it sure doesn’t look like it), and flush after every log line. Normally std::endl is a terrible idea… but flushing after every line is exactly the singular use case it’s actually intended for.
static std::map<MsgTy, std::string> msg_id{
    {MsgTy::OK, {"OK: "}},
    {MsgTy::WARNING, {"WARNING: "}},
    {MsgTy::ERROR, {"ERROR: "}},
    {MsgTy::CRITICAL, {"CRITICAL: "}}
};

As far as mapping enumerators to strings, this isn’t really the best way to go about it. It’s astoundingly inefficient and clunky for what should be a trivial task. A std::map is a heavyweight object… using it for literally 4 elements is… not a good usage.
A better solution is to either implement a to_string() function:
constexpr auto to_string(MsgTy mt)
{
    using namespace std::string_view_literals;

    switch (mt)
    {
    case MsgTy::OK:
        return "OK"sv;
    case MsgTy::WARNING:
        return "WARNING"sv;
    case MsgTy::ERROR:
        return "ERROR"sv;
    case MsgTy::CRITICAL:
        return "CRITICAL"sv;
    }
}

or to implement a stream inserter for the type:
template <typename CharT, typename Traits>
auto operator<<(std::basic_ostream<CharT, Traits>& o, MsgTy mt)
    -> std::basic_ostream<CharT, Traits>&
{
    switch (mt)
    {
    case MsgTy::OK:
        o << "OK";
    case MsgTy::WARNING:
        o << "WARNING";
    case MsgTy::ERROR:
        o << "ERROR";
    case MsgTy::CRITICAL:
        o << "CRITICAL";
    }

    return o;
}

or both.
Either option will be hundreds, if not thousands of times faster than using a std::map.
#define messaging(MsgTy, msg, log2file)

Okay, this is where the meat of the code is, and this is what you really want the focus to be on. Unfortunately, this is all wrong. This is exactly the way you should NEVER write a macro.
First let’s get the initial stuff out of the way. As I mentioned above, the convention in C++ (and even in C) is that macros should be in all caps. That’s not just for style, it’s because the unconstrained text replacement of the preprocessor is so dangerous. messaging isn’t exactly an uncommon word; it’s quite likely that it could be used for another identifer… with disastrous consequences. Using all caps accomplishes two things:

it alerts people to what they’re messing with; and
the only way it could every be used again is via a redefinition… which will trigger at least a warning.

The other problem with this preamble is that you’re using the message type enumeration’s type name as the parameter name. I can’t imagine why you’d think that’s a good idea. The only reason it works in this case is that you’re using an old-style enum. If you tried using a modern enum class, this whole macro would break.
There’s another issue buried in there: if the message type is ERROR or CRITICAL, you throw an exception. Okay, but the problem is the exception you throw is a std::string. If you run your program, it’s going to crash, because you catch a std::exception… but a std::string is not a std::exception. You probably want to either throw a std::runtime_error or, better, a custom exception type depending on whether it’s a ERROR or CRITICAL message.
Finally, you’ve made a critical macro error: you have repeated the argument(s). You have correctly wrapped them in parentheses, which helps prevent unexpected interactions with the surrounding code when expanded… but doesn’t help with the fact they they’re expanded multiple times. If you use an expression that changes the first argument in particular, who knows what could happen.
Overall, this is a terrible macro, for a number of reasons. First of all, it’s needlessly long. It injects almost 30 lines of code every time it’s used! In your main(), that try block that looks like it only has 4 lines in fact expands to well over 100 lines. That’s just ridiculous.
It’s also absurdly complex. Putting control flow in a macro is not just a “eh, it’s a thing you do”… it’s an absolute last resort. That’s really the golden rule of macros: keep them as simple as possible. That’s because they’re not only so hard to debug, but also because they’re expanded everywhere. They’re also exceptionally dangerous, so they should be written as simple as possible to avoid the need to ever tweak it in the future… as it is now, any time the requirements for how to log change, you have to mess with the macro code… which is playing with fire.
And a macro this complex just destroys your performance. Firstly, it will just absolutely trash your cache because all that machine code gets dumped everywhere the macro is used. (Although, if you’re lucky, and the macro is always used as you demonstrate, the compiler can probably remove most of those ifs.) But also, there are other side effects, too: for example, if messaging() were a function, profile-guided optimization would almost certainly mark the OK path as the hot path, and optimize the code accordingly… but that’s because there’s one if in one place; as a macro, that if gets repeated everywhere the macro is used, and it’s a different if every time, so PGO won’t help you much.
As it is, the macro’s code is in dire need of a rewrite, because it’s so repetitive, and there’s so much hard-coded in there (specifically the file name, over and over and over). But screwing with a macro is always a dangerous proposition; it’s MUCH riskier than refactoring a function. (It’s also sometimes much more frustrating, because the moment you touch a macro, you have to recompile everything, whereas a function can (sometimes!) be tweaked in isolation.)
And not only is it dodgy to use, hard to maintain, and inefficient… it’s also a terrible interface! Why is it necessary to specify whether you want the message written to the file or not on EVERY call? Isn’t assuming true a good default? With a function you could use an overload or a defaulted parameter for that, no problem.
At a bare minimum, to improve this macro, you should refactor as much as possible into functions:
#define MESSAGING(mt, msg, log_to_file) do { \
    auto const mt_ = (mt);

    if (mt_ == MsgTy::ok) \
        messaging_ok((msg), (log_to_file)); \
    else if (mt_ == MsgTy::warning) \
        messaging_warning((msg), (log_to_file)); \
    else if (mt_ == MsgTy::error) \
        messaging_error((msg), (log_to_file)); \
    else if (mt_ == MsgTy::critical) \
        messaging_critical((msg), (log_to_file)); \
} while (false)

Now you can fuss with the logic of each option safely.
Even better would be use static dispatch for this kind of thing. You could create a few types (ok_t, warning_t) and instances of those types (ok, warning), and then dispatch based on those:
struct ok_t {};
inline constexpr auto ok = ok_t{};
// etc. for the other message types

auto messaging(ok_t, std::string_view msg, bool log_to_file = true)
{
    std::cout << "OK: " << msg << std::endl; // endl to flush

    if (log_to_file)
    {
        auto out = std::ofstream{"log.txt", std::ios_base::app};
        out << "OK: " << msg;

        // or better yet, have a class that keeps the log file open
        // and just appends to it, rather than opening and closing
        // it repeatedly.
    }
}
// etc. for the other message types

messaging(ok, "Everything is good!");
messaging(warning, "Something isn't quite right!", false);
messaging(error, "Something went wrong!");
messaging(critical, "Something horribly went wrong!");

But that’s just one of dozens of techniques you can use to AVOID the use of macros… which is a much more useful skill to have in 2020.
In other words, all this has brought us back to the original point I wasn’t going to belabour. The best macro is the one you don’t write.
Questions
Are there any apparent issues with how I implemented the macro?
Yes, it is needlessly long and complex. Even for a non macro function, this is unnecessarily long and complex. It should be refactored into smaller functions for each of the four different behaviours.
Can there be any improvements made to it?
The best way to write a macro is: don’t.
I can’t conceive of why anyone would want to write a macro in 2020. Macros were a dirty hack when they were first created back in the 1970s. There may be a few very rare cases where you still need them, but by and large, if you can possibly solve a problem without macro, then THAT is the skill you should be exercising.
Is the intent clear on its usage?
Eeeh? Not really.
Is this the intended usage:
messaging(MsgTy::OK, 42, true);

Is this:
// won't work, but is it intended to?
messaging(MsgTy::OK, "a" << "b" << "c", true);

I'm also interested in any other feedback.
Don’t waste your time honing skills that nobody wants. Macros are old tech that are only, at best, tolerated, and only when there is absolutely no other option. The best skills you can learn regarding macros are ways to NOT use them. A programmer who is a master at writing macros, but because they don’t know all the ways they can avoid them always falls back to writing them, is less than useless to me on my projects.
Bottom line: Don’t waste your time. Getting good at writing macros helps no one. Instead, learn the techniques for AVOIDING macros. THOSE are skills that are actually useful in 2020 and beyond.


A function that reverses a string of characters My problem is in writing a function reverse(s) that reverses the character string s, a line at a time. My code here works, I write the line and it reverses the line. Is it a good solution?

#include <stdio.h>
#include <stdlib.h>
#define MAX 1000

void  reverse(char cad[] ,char cadenita[],int i);

int main() {
    int a ,i ;
    char cad[MAX] = {0,0};
    char cadenita[MAX];
    i= 0;
      while ((a = getchar()) != EOF) {
          if (a != '\n') {
             cad[i] = a;
              ++i;
          }
          else  {
              cad[i] = '\0' ;
             reverse(cad , cadenita, i);
              printf("%s\n",cadenita);
              i = 0;
          }
   }
    return (EXIT_SUCCESS);
}

void  reverse(char s[] ,char svol[], int i) {
    int a =0 ;
    // i is the amount characters in s

    while ((svol[a] = s[i-1]) != '\0' ){
        ++a;
        --i;
    }

}

Also in this part of code while ((svol[a] = s[i-1]) != '\0' ){ .
If I change to  ((svol[a] = s[i-1]) != '\n' ) it still works and gives the same result, but why? I think there is an issue. Can you explain? Also, where does it break?

Naming

The mix of Spanish and English is disconcerting: cad vs. s.  The use of cadenita for the reversed string also seems odd, since the reversed string is the same length as the original.  svol is even weirder, as s appears to be short for "string" (English) and vol, I'm guessing, is short for "volvió" (Spanish).  Keep in mind that the function's name, reverse, is English.  (Personally, I recommend using English everywhere.)

i is conventionally used as an iteration dummy variable rather than as a length parameter.  If you mean length, then call the parameter len or length.  Using size_t instead of int would also be a good hint to its purpose.

Function declaration

Does reverse() operate on NUL-terminated strings or on a byte array of a specified length?  You should make up your mind, since bugs tend to occur when the two conventions get mixed up.  If the function takes a length, then it shouldn't look for '\0'.  Alternatively, if looks for '\0', then it shouldn't accept a length parameter.

Since allocating memory is an annoyance in C, I would consider it idiomatic for the reverse() function to operate on the string in place rather than making a copy.  If the caller wants to keep the original string intact, then make it the caller's responsibility to duplicate the string first.

It is conventional to place main() last, so that you don't need to make forward declarations of the functions that it calls.

Input

I don't recommend reading a character at a time using getchar().  Character-at-a-time I/O is less efficient.  More importantly, using a more appropriate function to read an entire line can simplify your code.  Using getline(), for example, makes your intentions clearer, and it avoids the possibility of a buffer overflow.

#include <stdio.h>

char *reverse(char *s, size_t len) {
    for (int i = 0, j = len - 1; i < j; i++, j--) {
        char swap = s[i];
        s[i] = s[j];
        s[j] = swap;
    }
    return s;
}

int main() {
    char *line = NULL;
    size_t linecap = 0;
    ssize_t linelen;
    while ((linelen = getline(&line, &linecap, stdin)) > 0) {
        if (line[linelen - 1] == '\n') {
            line[--linelen] = '\0';
        }
        puts(reverse(line, linelen));
    }
}


Generate HTML from JSON I have to generate a lot of tables from different JSON files (up to 20 tables per page). Because it's a lot of data, I really want to keep loading speed in mind. I know it's better to use a non jQuery solution, but native is not in my skillset right now.

$(document).ready(function(){

  (function getPersonData(){
   $.getJSON('path/to/json', function(data){

  (function addPersonsTable1(personsData){

    var elementContainer = '';

    $.each(personsData.persons, function(key, person){
      elementContainer = elementContainer + '
' +
        '' + person.value1 + '	' +
        '' + person.value2 + '	' +
        '' + person.value3 + '	' +
        '' + person.value4 + '	' +
        '' + person.value5 + '	' +
      '
';
    });

    $('.persons-table-1').append(elementContainer);

  }(data));

  (function addPersonsTable2(personsData){

    var elementContainer = '';

    $.each(personsData.persons, function(key, person){
      elementContainer = elementContainer + '
' +
        '' + person.value1 + '	' +
        '' + person.value2 + '	' +
        '' + person.value3 + '	' +
        '' + person.value4 + '	' +
        '' + person.value5 + '	' +
      '
';
    });

    $('.persons-table-2').append(elementContainer);

  }(data));

  /*
  *
  *
  *
  *
  *  <..... And many more tables
  *
  *
  *
  *
  */

 }).done(function(){
  loadedAnimation();
});

});

Use detach to remove the elements from the DOM, append the tables to the detached nodes, then add them back to to DOM:

var table = $( "#myTable" );
var parent = table.parent();

table.detach();

 // ... add lots and lots of rows to table

parent.append( table );

References

Detach Elements to Work with Them
jQuery API: detach


Filling an array with the same data at a regular interval without a for loop I have some code which generates the coordinates of a cylindrically-symmetric surface, with coordinates given as \$(r, \theta, \phi)\$.  At the moment, I generate the coordinates of one \$\phi\$ slice, and store that in a \$2 \times N\$ array (for \$N\$ bins), and then in a for loop I copy this array for each value of \$\phi\$ from \$0\$ to \$2\pi\$:

import numpy as np

# this is the number of bins that my surface is chopped into
numbins = 50

# these are the values for r
r_vals = np.linspace(0.0001, 50, numbins, endpoint = True)

# these are the values for theta
theta_vals = np.linspace(0, np.pi / 2, numbins, endpoint = True)

# I combine the r and theta values into a 2xnumbins array for one "slice" of phi
phi_slice = np.vstack([r_vals,theta_vals]).T

# this is the array which will store all of the coordinates of the surface
surface = np.zeros((numbins**2,3))
lower_bound = 0
upper_bound = numbins

# this is the size of the bin for phi
dphi = (2 * np.pi) / numbins

# this is the for loop I'd like to eliminate.
# For every value of phi, it puts a copy of the phi_slice array into
# the surface array, so that the surface is cylindrical about phi.
for phi in np.linspace(0, (2 * np.pi) - dphi, numbins):
    surface[lower_bound:upper_bound, :2] = phi_slice
    surface[lower_bound:upper_bound,2] = phi
    lower_bound += numbins
    upper_bound += numbins

I'm calling this routine in a numerical integration of 1e6 or 1e7 steps, and while numbins is 50 in the example above, in practice it'll be in the thousands.  This for loop is a choking point, and I'd really like to eliminate it to speed things up.  Is there a good NumPythonic way to do the same thing as this for loop?

For starter, I would suggest making a function out of this pile of code so its easier to read an reason about:

def create_surface(bins_count=50, radius_start=0.0001, radius_end=50):
    radii = np.linspace(radius_start, radius_end, bins_count, endpoint=True)
    thetas = np.linspace(0, np.pi / 2, bins_count, endpoint=True)

    coordinates = np.vstack([radii, thetas]).T

    surface = np.zeros((bins_count**2, 3))
    lower_bound = 0
    upper_bound = numbins

    # this is the size of the bin for phi
    delta_phi = (2 * np.pi) / bins_count

    for phi in np.linspace(0, (2 * np.pi) - delta_phi, bins_count):
        surface[lower_bound:upper_bound, :2] = coordinates
        surface[lower_bound:upper_bound, 2] = phi
        lower_bound += bins_count
        upper_bound += bins_count
    return surface

Optionnally, throwing in a docstring here wouldn't hurt.

Now this function has two purposes: compute the coordinates and build a surface out of it, let's split things up.

def build_coordinates(bins_count=50, radius_start=0.0001, radius_end=50):
    radii = np.linspace(radius_start, radius_end, bins_count, endpoint=True)
    thetas = np.linspace(0, np.pi / 2, bins_count, endpoint=True)

    return np.vstack([radii, thetas]).T

def create_surface(coordinates):
    bins_count, two = coordinates.shape
    assert two == 2

    surface = np.zeros((bins_count**2, 3))
    lower_bound = 0
    upper_bound = numbins

    # this is the size of the bin for phi
    delta_phi = (2 * np.pi) / bins_count

    for phi in np.linspace(0, (2 * np.pi) - delta_phi, bins_count):
        surface[lower_bound:upper_bound, :2] = coordinates
        surface[lower_bound:upper_bound, 2] = phi
        lower_bound += bins_count
        upper_bound += bins_count
    return surface

Usage being phi_slice = build_coordinates(50); surface = create_surface(phi_slice). With the advantage of being able to build several surfaces out of the same coordinates.

Now let's start to look into removing that for loop. So basicaly, the surface you want to build should look like:

[[r0, Θ0, φ0],
 [r1, Θ1, φ0],
 ...
 [rn, Θn, φ0],
 [r0, Θ0, φ1],
 [r1, Θ1, φ1],
 ...
 [rn, Θn, φ1],
 ...
 [r0, Θ0, φn],
 [r1, Θ1, φn],
 ...
 [rn, Θn, φn]]

Where

[[r0, Θ0],
 [r1, Θ1],
 ...
 [rn, Θn]]

are the coordinates (your phi_slice) and

[φ0, ..., φn]

are np.linspace(0, (2 * np.pi) - dphi, numbins).

So better build surface by repeating phi_slice and the elevations enought time and then concatenating them together rather than copy/pasting them into a blank array.

This is rather simple since numpy provides np.repeat and np.concatenate. But both have their specifics:

np.repeat return a flat array when no axis is given and repeat each element the requested amount of time before starting to repeat the next one. This is exactly what is needed for the elevations:

>>> np.repeat(np.array([φ0, φ1, φ2]), 3)
array([φ0, φ0, φ0, φ1, φ1, φ1, φ2, φ2, φ2])

But not quite so for the coordinates:

>>> np.repeat(np.array([[r0, Θ0], [r1, Θ1], [r2, Θ2]]), 2, axis=0)
array([[r0, Θ0],
       [r0, Θ0],
       [r1, Θ1],
       [r1, Θ1],
       [r2, Θ2],
       [r2, Θ2]])

Instead, we would like to repeat the whole block of coordinates at once. So let's add a dimension to repeat along it and then reshape the result to remove the extra dimension:

>>> coordinates = np.array([[r0, Θ0], [r1, Θ1], [r2, Θ2]])
>>> np.repeat(coordinates[np.newaxis, :, :], 2, axis=0).reshape(6, 2)
array([[r0, Θ0],
       [r1, Θ1],
       [r2, Θ2],
       [r0, Θ0],
       [r1, Θ1],
       [r2, Θ2]])

Where 6 is the length of axis 0 times the amount of repetitions.

The last step being to combine both repeated arrays using np.concatenate. However concatenate requires that both array have the same amount of axis so we need to add one to the elevation vector:

>>> elevations = np.repeat(np.array([φ0, φ1, φ2]), 3)
>>> coordinates = np.array([[r0, Θ0], [r1, Θ1], [r2, Θ2]])
>>> repeated_coordinates = np.repeat(coordinates[np.newaxis, :, :], 3, axis=0).reshape(9, 2)
>>> np.concatenate((repeated_coordinates, elevations[:, np.newaxis]), axis=1)
array([[r0, Θ0, φ0],
       [r1, Θ1, φ0],
       [r2, Θ2, φ0],
       [r0, Θ0, φ1],
       [r1, Θ1, φ1],
       [r2, Θ2, φ1],
       [r0, Θ0, φ2],
       [r1, Θ1, φ2],
       [r2, Θ2, φ2]])

This yield:

def build_coordinates(bins_count=50, radius_start=0.0001, radius_end=50):
    radii = np.linspace(radius_start, radius_end, bins_count, endpoint=True)
    thetas = np.linspace(0, np.pi / 2, bins_count, endpoint=True)

    return np.vstack([radii, thetas]).T

def create_surface(coordinates):
    bins_count, two = coordinates.shape
    assert two == 2

    elevation = np.linspace(0, 2 * np.pi * (1 - 1/bins_count), bins_count)
    elevations = np.repeat(elevation, bins_count)
    coords = np.repeat(coordinates[np.newaxis, :, :], bins_count, axis=0).reshape(bins_count**2, 2)

    return np.concatenate((coords, elevations[:, np.newaxis]), axis=1)


Create objects corresponding to whichever field has value from among a list of fields I have to create a form, which enables users to move stock of items to different statuses. The form will show the current stock(quantity) in a given status and then 5 input fields corresponding to 5 statuses. The user can input into one or more fields as to the quantity that they want to be moved into the new status. Once the user submits the form, based on whether a field has value, I have to put an entry corresponding to that entry into a table which will store some common properties as well as the intended status and the quantity of each.

Consider the following scenario, if total stock in status Purchased is 100.

Production  - 30
Finished - 20
Finished (Internal Only)
Wastage
Base Stock - 50

I will need to create 3 entries corresponding to Production, Finished and Base Stock, respectively from a single object returned from the front end form.

The application is written in AngularJS and C# Web API. The following is what I have written so far. WStockStatus is a single object in which I capture the input values from front end as separate fields.

Does this follow best practices? Is there a better way to go about it?

public bool moveStock(WStockStatus fromStockStatus)
    {
    const int STATUS_BASE = 1;
    const int STATUS_FIN = 2;
    const int STATUS_FINI = 3;
    const int STATUS_WAST = 4;
    const int STATUS_PRGR = 5;
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;
    if(fromStockStatus.qtyBase > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyBase;
        stockStatus.status = STATUS_BASE;
        createStockStatus(stockStatus);  //another function that creates individual entry for stock status
    }
    if(fromStockStatus.qtyFin > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyFin;
        stockStatus.status = STATUS_FIN;
        createStockStatus(stockStatus);
    }
    if(fromStockStatus.qtyFini > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyFini;
        stockStatus.status = STATUS_FINI;
        createStockStatus(stockStatus);
    }
    if(fromStockStatus.qtyWast > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyWast;
        stockStatus.status = STATUS_WAST;
        createStockStatus(stockStatus);
    }
    if(fromStockStatus.qtyPrgr > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyPrgr;
        stockStatus.status = STATUS_PRGR;
        createStockStatus(stockStatus);
    }
    return true;
}

    private void createStockStatus(StockStatus newObj)
    {
        if (newObj.isValid())
        {
            newObj.deleted = false;
            newObj.submittedId = null;
            UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
            newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
            context.StockStatus.Add(newObj);
            context.SaveChanges();
        }
        return null;
    }

This

const int STATUS_BASE = 1;
const int STATUS_FIN = 2;
const int STATUS_FINI = 3;
const int STATUS_WAST = 4;
const int STATUS_PRGR = 5;

screams for an enum like

public enum StockState
{
    Base = 1, Fin, Fini, Wast, Prgr
}

but you should rename the enumvalues in a way that you or Sam the maintainer doesn't has to guess about the meaning. If you or Sam have to fix a bug in 6 months it will be easier if your code can be graps at first glance. Just don't use abbreviations for naming things.

bool moveStock()

Based on the .NET Naming Guidlines methods should be named using PascalCase casing hence moveStock() should read MoveStock(). While we are at naming things I would name the method parameter sourceStockState which is IMO more clear.

stockStatus.date again properties should be named using PascalCase casing.

Because moveStock can't return false it should be void.

Let's get the meat. If you would change the createStockStatus() method in a way that it takes 2 additional parameters your moveStock() would get more readable because it would get shorter.

Assume the createStockStatus() would look like so

private void createStockStatus(StockStatus newObj, int quantityUOM, StockState state)
{
    if (!newObj.isValid()) { return; }

    newObj.quantityUOM = quantityUOM;
    newObj.state = (int)state;
    newObj.deleted = false;
    newObj.submittedId = null;
    UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
    newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
    context.StockStatus.Add(newObj);
    context.SaveChanges();
}

then the former moveStock() could look like so

public void moveStock(WStockStatus fromStockStatus)
{
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;

    if(fromStockStatus.qtyBase > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyBase, StockState.Base);
    }
    if(fromStockStatus.qtyFin > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyFin, StockState.Fin);
    }
    if(fromStockStatus.qtyFini > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyFini, StockState.Fini);
    }
    if(fromStockStatus.qtyWast > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyWast, StockState.Wast);
    }
    if(fromStockStatus.qtyPrgr > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyPrgr, StockState.Prgr);
    }
}

Edit

  But my main intention was to see if there is a better way by which I can avoid using these multiple if statements, like using a different data structure or use a different format of capturing the data.

By moving the if condition into createStockStatus() like so

private void createStockStatus(StockStatus newObj, int quantityUOM, StockState state)
{
    if (quantityUOM <= 0 || !newObj.isValid()) { return; }

    newObj.quantityUOM = quantityUOM;
    newObj.state = (int)state;
    newObj.deleted = false;
    newObj.submittedId = null;
    UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
    newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
    context.StockStatus.Add(newObj);
    context.SaveChanges();
}

the moveStock() could look like so

public void moveStock(WStockStatus fromStockStatus)
{
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;

    createStockStatus(stockStatus, fromStockStatus.qtyBase, StockState.Base);
    createStockStatus(stockStatus, fromStockStatus.qtyFin, StockState.Fin);
    createStockStatus(stockStatus, fromStockStatus.qtyFini, StockState.Fini);
    createStockStatus(stockStatus, fromStockStatus.qtyWast, StockState.Wast);
    createStockStatus(stockStatus, fromStockStatus.qtyPrgr, StockState.Prgr);

}