Angular2/4 broadcast service using RxJS I am no expert in RxJS as I am still learning my way around the new paradigm of Observable/Subject.

I was trying to implement a broadcast service (based on NgRadio) that allows a new subscriber to get the nth last value of events he was listening to and the next emitted values after subscription.

As I stumbled upon issues with ReplaySubject to ask for the last n values... I ended up implementing it as follows. I feel that the code is not that crisp and clean so I would appreciate any improvement from more experienced RxJS developers.

import { BehaviorSubject } from 'rxjs/BehaviorSubject';
import { Observable } from 'rxjs/Observable';
import { ReplaySubject } from 'rxjs/ReplaySubject';
import { Subject } from 'rxjs/Subject';
import * as _ from 'lodash';
export interface RadioEvent {
    key: string;
    data?: any;
}

export class RadioService {
    private separator = ':';
    private _eventBus = new Subject<RadioEvent>();
    private _eventCacheBus = new ReplaySubject<RadioEvent>();

    keyMatch(key, wildcard) {
        var w = '*';
        var ww = '**';
        var partMatch = function (wl, k) {
            var match = (wl === w) || (wl === k);
            return match;
        };
        var sep = this.separator;
        var kArr = key.split(sep);
        var wArr = wildcard.split(sep);
        var kLen = kArr.length;
        var wLen = wArr.length;
        var max = Math.max(kLen, wLen);
        for (var i = 0; i < max; i++) {
            var cK = kArr[i];
            var cW = wArr[i];
            // '**' match all gragments
            if (cW == ww && (typeof cK !== 'undefined')) {
                return true;
            }
            // test if fragments match
            if (!partMatch(cW, cK)) {
                return false;
            }
        }
        return true;
    };

    cast<T>(key: string, data?: any) {
        if (typeof key !== 'string' || !key.length) {
            throw 'Bad key. Please provide a string';
         }

        this._eventBus.next({ key: key, data: data });
        this._eventCacheBus.next({ key: key, data: data });
    };

    on<T>(key: string, count?: number) {
        var _this = this;
        var normalobs = this._eventBus
        .filter(function (event: RadioEvent) {
            return _this.keyMatch(event.key, key);
        }).map(function (event) {
            return event.data;
        });
        if (_.isNil(count)) {
            return normalobs;
        } else {
            let obs = this._eventCacheBus
            .filter(function (event: RadioEvent) {
                return _this.keyMatch(event.key, key);
            }).map(function (event) {
                return event.data;
            });
            let subject = new ReplaySubject<T>(count);
            obs.subscribe(value => {
                subject.next(value);
            })
            return Observable.merge(normalobs, subject).distinct();
        }
    }
}

First of all, I recommend you to ensure your code is correct (see my comment on your question).

Here are a few suggestions about improving your code.

Please do not use non-informative names like w, ww, sep, kArg, cW, CK, and even the "well-known" i, j, k, m, n. They are absolutely horrible. They make decent code be an unreadable puzzle. They hide the intent and make me think where I should not be.
Please always make sure you use Type when a) the compiler loses (or is unable to infer) the type information; b) you can narrow down the type. That includes the function return type. Typing is not panacea but it helps prevent an entire class of bugs.
Do not use generic methods smth<T>(...) unless you use the type T information anywhere in the function.
Minimize the distance between variable declaration, assignment, and first usage. Better declare and assign in the same place. Ideally, find a way to not even declare a variable.
In error messages be more specific about what went wrong.

  const star = '*';
  const twoStars = '**';

  export class RadioService {
    private separator = ':';
    private _eventBus = new Subject<RadioEvent>();
    private _eventCacheBus = new ReplaySubject<RadioEvent>();

    keyMatch(keyList: string, wildcardList: string): boolean {
      const isMatch = (wildcard, key) => (wildcard === star) || (wildcard === key);

      const allKeys = keyList.split(this.separator);
      const allWildcards = wildcardList.split(this.separator);

      const keyWildcardPairs = allKeys
        .reduce((accum, current, index) => {
          accum.push([current, allWildcards[index]]);
          return accum;
        }, <string[][]>[]);

      return keyWildcardPairs.some(pair => {
        const key = pair[0];
        const wildcard = pair[1];
        // '**' match all gragments
        if (wildcard == twoStars && (typeof key !== 'undefined')) {
          return true;
        }
        // test if fragments match
        if (!isMatch(wildcard, key)) {
          return true;
        }
        return false;
      });
    };

    cast(key: string, data?: any): void {
      if (typeof key !== 'string' || key.length <= 0) {
        throw `Bad key '${JSON.stringify(key)}'. Please provide a non-empty string.`;
      }

      this._eventBus.next({ key: key, data: data });
      this._eventCacheBus.next({ key: key, data: data });
    };

    on(key: string, count?: number): Observable<any> {
      const _this = this;

      const normalObservable = this._eventBus
        .filter(event => _this.keyMatch(event.key, key))
        .map(event => event.data);

      const withoutReplay = _.isNil(count);
      if (withoutReplay)
        return normalObservable;

      const subject = new ReplaySubject<number>(count);
      this._eventCacheBus
        .filter(event => _this.keyMatch(event.key, key))
        .map(event => event.data)
        .subscribe(value => subject.next(value));

      return Observable
        .merge(normalObservable, subject)
        .distinct();
    }
  }


RSVP.hash replacement using ES6 Promise We have a pretty big code base that made use of RSVP which we are moving to node 4.3.2+. The only RSVP feature we use frequently that's not in the native js Promise api is the hash() function. I expected to either use the RSVP source or find something on stack overflow. Most people appear to be be solving the problem with different constraints & requirements which makes existing solutions I found somewhat complex so I whipped up my own:

function hash (hashOfPromises) {
  var keys = Object.keys(hashOfPromises);
  return Promise.all(keys.map(function (key) {
    return hashOfPromises[key];
  })).then(function (list) {
    return list.reduce(function (hashOfResolved, value, i) {
      hashOfResolved[keys[i]] = value;
      return hashOfResolved;
    }, {});
  });
}

I think this is pretty good. I would maybe replace the word "list" with "values".

That said, I think you can simplify this code a bit if you embrace newer versions of js, and perhaps with the use of lodash.

Just a quick example of what you could do:

const _ = require('lodash');

async function hash(hashOfPromises){
    let keys = Object.keys(hashOfPromises);
    let promises = keys.map(key => hashOfPromises[key]);
    let values = await Promise.all(promises);

    return _.zipObject(keys, values);
}


Simple pattern matching between two string inputs using Java (Google interview challenge) The interview question is:

Write a function isMatch (without using java.util.Regex) that takes two strings as arguments:

s (a string to match), and
p (a regular expression pattern).

It should return a Boolean denoting whether s matches p.
p can be composed of alphabetic characters and '*' (denoting that the previous character matches 0 or more times).

This is the code I have written:
    private boolean isNextCharStarred(String s) {
         return s.length() >= 2 && s.charAt(1) == '*';
     }

     private boolean recIsMatch(String s, String p, boolean hasPatternBeenEaten) {
         if (p.length() == 0) {
             return true;
         }
         if (s.length() == 0) {
             if (isNextCharStarred(p)) {
                 return recIsMatch(s, p.substring(2), true);
             }
             return p.length() == 0;
         }
         if (isNextCharStarred(p)) {
             if (s.charAt(0) != p.charAt(0)) {
                 return recIsMatch(s, p.substring(2), true);
             }
             return recIsMatch(s.substring(1), p, hasPatternBeenEaten)
|| recIsMatch(s, p.substring(2), true);
         }
         if (s.charAt(0) == p.charAt(0)) {
             return recIsMatch(s.substring(1), p.substring(1), true);
         }
         if (hasPatternBeenEaten) {
             return false;
         }
         return recIsMatch(s.substring(1), p, hasPatternBeenEaten);
     }

     public boolean isMatch(String s, String p) {
         return recIsMatch(s, p, false);
     }

Are there any bugs? How could it be improved?

Empty pattern should only match empty text

isMatch("aa", "a") returns true, when it should be false.
The problem is with this condition:

 if (p.length() == 0) {
     return true;
 }

An empty pattern should match only an empty text:

 if (p.isEmpty()) {
     return s.isEmpty();
 }

Notice that I replaced s.length() == 0 with s.isEmpty().
This is a more natural way to check if a string is empty.

Use better variable names

Instead of s and p it would be more natural to call them text and pattern.

The function isNextCharStarred is always used on the pattern,
but its parameter is called s, which is the same name as the variable used for text in the other function. Even p would have been less confusing. pattern would be nice and natural.

Eliminate hasPatternBeenEaten

The parameter hasPatternBeenEaten looked a bit suspicious to me.
Let's take a closer look.

The value of the variable is only read in one place in the code.
Let's look at the piece code just before that:

 if (isNextCharStarred(p)) {
     // will always return from this block
 }
 if (s.charAt(0) == p.charAt(0)) {
     return ...
 }
 if (hasPatternBeenEaten) {
     return false;
 }

That is:

If the pattern starts with X* (where X is any letter), the first if block will return.
If the pattern doesn't start with X*, and the first characters match, the second if block will return.
If the pattern doesn't start with X*, and the first characters don't match, then we need not look further: the pattern doesn't match, we can return false.

Therefore, the hasPatternBeenEaten is unnecessary, and can be safely eliminated.

Simplify

With the above changes, the code will look more like this:

  private boolean isMatch(String text, String pattern) {
    if (pattern.isEmpty()) {
      return text.isEmpty();
    }

    if (text.isEmpty()) {
      if (isNextCharStarred(pattern)) {
        return isMatch(text, pattern.substring(2));
      }
      return pattern.isEmpty();
    }

    if (isNextCharStarred(pattern)) {
      if (text.charAt(0) != pattern.charAt(0)) {
        return isMatch(text, pattern.substring(2));
      }
      return isMatch(text.substring(1), pattern)
        || isMatch(text, pattern.substring(2));
    }

    if (text.charAt(0) == pattern.charAt(0)) {
      return isMatch(text.substring(1), pattern.substring(1));
    }

    return false;
  }

This can probably be written simpler, by rearranging the conditions and extracting common patterns.

First of all the second pattern.isEmpty() condition is redundant,
because at that point we already know that pattern is not empty,
due to the very first condition in the method.

Next, we can extract the common condition text.charAt(0) == pattern.charAt(0) to a variable firstMatch, and flatten the if conditions to direct return statements, like this:

if (pattern.isEmpty()) {
  return text.isEmpty();
}

if (text.isEmpty()) {
  return isNextCharStarred(pattern) && isMatch(text, pattern.substring(2));
}

boolean firstMatch = text.charAt(0) == pattern.charAt(0);

if (isNextCharStarred(pattern)) {
  return firstMatch && isMatch(text.substring(1), pattern)
    || isMatch(text, pattern.substring(2));
}

return firstMatch && isMatch(text.substring(1), pattern.substring(1));

Lastly, we can generalize firstMatch to include the case of empty text,
which will make the second if redundant:

if (pattern.isEmpty()) {
  return text.isEmpty();
}

boolean firstMatch = !text.isEmpty() && text.charAt(0) == pattern.charAt(0);

if (isNextCharStarred(pattern)) {
  return firstMatch && isMatch(text.substring(1), pattern)
    || isMatch(text, pattern.substring(2));
}

return firstMatch && isMatch(text.substring(1), pattern.substring(1));


Survivor programming challenge I am working on a survivor problem:

  Complete Question Text: Take a second to imagine that you are in a
  room with 100 chairs arranged in a circle. These chairs are numbered
  sequentially from One to One Hundred.

  At some point in time, the person in chair #1 will be told to leave
  the room. The person in chair #2 will be skipped, and the person in
  chair #3 will be told to leave. Next to go is person in chair #6. In
  other words, 1 person will be skipped initially, and then 2, 3, 4..
  and so on. This pattern of skipping will keep going around the circle
  until there is only one person remaining.. the survivor. Note that the
  chair is removed when the person leaves the room.

  Write a program to figure out which chair the survivor is sitting in.

Below is the code I have:

public class ChairProblem {

    public static void main(String[] args) {
        System.out.println(getSurvivors(100));
    }

    private static int getSurvivors(int numChairs) {
        if (numChairs < 1) {
            return -1;
        }

        // populate chair array list
        ArrayList<Integer> chairs = new ArrayList<Integer>();
        for (int i = 0; i < numChairs; i++) {
            chairs.add(i + 1);
        }

        // removing all but one elements
        int indexOfChair = 0;
        int count = 1;
        while (chairs.size() > 1) {
            chairs.remove(indexOfChair);
            indexOfChair += count;// skip the count number of chairs
            count++; //increase the number of chairs to skip by 1
            indexOfChair %= chairs.size();// loop to beginning if necessary
        }

        return chairs.get(0);
    }
}

How can this be improved? I was asked this question in an interview and I gave this answer, but I'm not sure whether this can be improved in terms of complexity or Java Docs as well.

Basics
In Java, since version 7, there is no need to specify the generic type of a generic class on the right-hand side of the initializer. Instead, you can use the "diamond operator". The following:

    ArrayList<Integer> chairs = new ArrayList<Integer>();

should be:
    ArrayList<Integer> chairs = new ArrayList<>();

Additionally, unless you have a specific need, you should use the highest level of abstraction that's useful for your variables. There is no need to declare chairs as an ArrayList, when a List would be fine:
    List<Integer> chairs = new ArrayList<>();

Data Types
Here you have used an ArrayList, but this is a bad choice. Performance in ArrayLists is poor when you add or remove items from the middle of the list. A LinkedList is generally better for that type of operation.
Alternate solution
A linked list is a great option for this problem, because it can be made to behave like a circle. What we do, is start with a populated list, and then "walk the list". We remove the first member, then take the next member, and move them to the end of the list. Then we remove the third member, and the 4th and 5th survive and go to the end. Note that the first, second, and third survivors (chair 2, 4, and 5) are now next to each other at the end.... (after chair 100).
Note that for every member we remove from the list, we add a bunch of survivors  to the end. By doing some modulo arithmetic, the amount we shift to the end can be controlled to the size of the remaining members.
We repeat the process until the list is size() == 1
private static int getSurvivorsLL(final int numChairs) {
    Deque<Integer> chairs = new LinkedList<>();
    while (chairs.size() < numChairs) {
        chairs.add(chairs.size() + 1);
    }
    int skip = 0;
    while (chairs.size() > 1) {
        System.out.println("Eliminated " + chairs.removeFirst());
        skip++;
        int shift = skip % chairs.size();
        for (int i = 0; i < shift; i++) {
            chairs.addLast(chairs.removeFirst());
        }
    }
    return chairs.removeFirst().intValue();
}

Note that, because I need the removeFirst() method, I use the Deque personality for the LinkedList.
Update - Neat, Custom, or Fast
This question got me thinking, for both good, and bad reasons. I initially misread the question, and gave a broken answer. Then I "improved" my alternative suggestion to be a more natural language fit than an ArrayList (which does not have O(1) "remove" time).
Unfortunately, for me, I then ran a benchmark against my code, and the OP's code. My code lost, even though the LinkedList is a more natural fit for this problem. It just makes sense that the LinkedList should be faster... all we are doing is shuffling items one-at-a-time to the end of the list. and there's only one item moved each turn. So, why was it slow? To put things in perspective, here are the times of the OP's code:
Task Chairs -> OP: (Unit: MICROSECONDS)
  Count    :   1000000      Average  :    2.3310
  Fastest  :    1.9730      Slowest  : 1878.1240
  95Pctile :    2.7630      99Pctile :    3.9480
  TimeBlock : 2.506 2.323 2.245 2.479 2.268 2.262 2.265 2.262 2.386 2.319
  Histogram : 981012 14462  4176   229    97    13     5     3     1     2

It computes the solution for 100 chairs in under 2 microseconds. But the time for the LinkedList solution I propose is:
Task Chairs -> LL: (Unit: MICROSECONDS)
  Count    :    182906      Average  :   16.4010
  Fastest  :   13.8150      Slowest  : 2252.3280
  95Pctile :   23.6840      99Pctile :   30.7890
  TimeBlock : 20.028 16.870 18.086 16.485 15.615 15.323 15.411 15.385 15.399 15.417
  Histogram : 179027  3537   253    52    15     2    16     4

where the fastest time is in about 14 mircoseconds - 7 times slower than the OP code, even though it in theory does less work!.
Right, so, what would make the code faster? First up, I designed a custom node class that would be simpler than a full Linked List. Here is the code:
private static final class Chair {
    private final int id;
    private Chair next = null;

    public Chair(int id) {
        this.id = id;
    }

}

private static int getSurvivorsCL(final int numChairs) {
    Chair previous = buildCircle(numChairs);
    int size = numChairs;
    while (size > 1) {
        Chair togo = previous.next;
        previous.next = togo.next;
        togo.next = null;
        size--;
        int shift = (numChairs - size) % size;
        while (shift-- > 0) {
            previous = previous.next;
        }
    }
    return previous.id;
}

private static Chair buildCircle(int numChairs) {
    final Chair last = new Chair(numChairs);
    Chair tmp = last;
    while (--numChairs > 0) {
        Chair c = new Chair(numChairs);
        c.next = tmp;
        tmp = c;
    }
    last.next = tmp;
    return last;
}

The above code is a 'clean' version of what I would expect the circular chair arrangement to accomplish. What is the time for that?
Task Chairs -> CL: (Unit: MICROSECONDS)
  Count    :     943355      Average  :     3.1800
  Fastest  :     2.3680      Slowest  : 34884.2530
  95Pctile :     3.5520      99Pctile :     3.9480
  TimeBlock : 3.243 3.098 3.068 3.160 3.478 3.202 3.241 3.047 3.127 3.137
  Histogram : 936200  2336  4519   169    84    29     3     1     2     4     4     2     1     1

This is 4 times faster than the LinkedList, but, it is still 50% slower than the OP's code? The 4-times faster than LinkedList is impressive, but there's still something that does not make sense to me.... it should be faster than ArrayList.
So, to experiment, I used an even simpler approach of a static array, where none of the data is moved at all. The only thing updated is a "pointer" to the next chair. In other words, the index in the array is essentially the chair number, and the value in the array is the "next" chair. This way we can create a logical circle, and just change a pointer each time a chair is removed. Here is the code:
private static int getSurvivorsAN(final int numChairs) {
    int[] chairs = new int[numChairs];
    for (int i = 1; i <= numChairs; i++) {
        chairs[i - 1] = i % numChairs;
    }
    int current = numChairs - 1;
    int skip = 0;
    int size = numChairs;
    while (current != chairs[current]) {
        int remove = chairs[current];
        chairs[current] = chairs[remove];
        size--;
        skip++;
        int loopskip = skip % size;
        while (--loopskip >= 0) {
            current = chairs[current];
        }
    }
    // chairs are 0-based, we need to return 1-based, so add 1.
    return current + 1;
}

How fast was that?
Task Chairs -> AN: (Unit: MICROSECONDS)
  Count    :     939602      Average  :     3.1920
  Fastest  :     2.7620      Slowest  : 18780.0630
  95Pctile :     3.5530      99Pctile :     3.9470
  TimeBlock : 3.271 3.145 3.127 3.181 3.195 3.141 3.140 3.152 3.175 3.401
  Histogram : 934304   761  4360   114    47     9     3     2     0     1     0     0     1

About the same as the Chair node version above... but, still slower than the OP.
So, how to beat the OP? Well, it must boil down to the fact that a System.arrayCopy() of all the "remaining" chairs in the circle (what should be an O(n) operation, where n is the size of the List, is more efficient than looping over each value in the skip. So, for example, on the first iteration, we remove chair 1, it must mean that copying every other chair "back one spot" is faster than just moving 1 chair to the end. To test this, I made an efficient version of ArrayList (i.e. using primitives, not Integer). Here's the code:
private static int getSurvivorsAS(final int numChairs) {
    int[] chairs = new int[numChairs];
    for (int i = 0; i < numChairs; i++) {
        chairs[i] = i + 1;
    }

    int remove = 0;
    int skip = 0;
    int size = numChairs;
    while (size > 1) {
        size--;
        System.arraycopy(chairs, remove + 1, chairs, remove, size - remove);
        chairs[size] = 0;
        skip++;
        remove = (remove + skip) % size;
    }
    return chairs[0];
}

How does this compare?
Task Chairs -> AS: (Unit: MICROSECONDS)
  Count    :   1000000      Average  :    1.6140
  Fastest  :    1.1840      Slowest  : 1699.3120
  95Pctile :    1.9740      99Pctile :    2.3690
  TimeBlock : 1.653 1.614 1.573 1.689 1.593 1.591 1.592 1.583 1.650 1.606
  Histogram : 970982 26003   384  2495    78    35    17     4     1     0     1

So, what does this all mean?
Well, it means that System.arraycopy() is fast. How do things scale, though?
Here are the scaling charts for three of the code blocks, the OP code, the best Circular List code (with the Chair node), and the ArrayShift code.
All three have a O(n^2) type complexity, so it all washes out, essentially. Note that the array-shift code is fastest for all tested scales (all the way up to more than 100,000 chairs.
Conclusion
The OP's code is better than I expected in terms of performance. The LinkedList code is worse than I expected. The best code from a readability perspective is, I think, the LinkedList code. It best represents the actual problem.
The best code from a performance perspective is a primitive array-of-int which you shift using System.arrayCopy() in order to remove chairs.


Quadratic expression calculator I'm hoping to get some feedback on how to improve and optimise a program I wrote which takes three values as input (via the terminal) and uses them in the quadratic formula.

I'm aware this is mostly micro-optimisation, but any advice whatsoever would be much appreciated.

public class Quadratic_Equations {
    static Scanner sc = new Scanner (System.in);

    public static void main(String[] args) {
        while (true) {
            System.out.println("Please enter a, b and c or enter -1 at any time to exit");
            double a = tryParse(sc.nextLine());
            a = checkIfValidNumber(a);
            if (a == -1)
                break;
            double b = tryParse(sc.nextLine());
            b = checkIfValidNumber(b);
            if (b == -1)
                break;
            double c = tryParse(sc.nextLine());
            c = checkIfValidNumber(c);
            if (c == -1)
                break;

            System.out.println(useQuadraticFormula(a, b, c));
            System.out.println();
        }
        sc.close();
    }

    private static double checkIfValidNumber(double number) {
        while (number == -1.0) {
            System.out.println("You didn't enter a valid number for a. Please try again.");
            number = tryParse(sc.nextLine());
        }
        return number;
    }

    public static Double tryParse(String text) {
        try {
            return new Double(text);
        } catch (NumberFormatException e) {
            return -1.0;
        }
    }

    public static String useQuadraticFormula(double a, double b, double c) {
        double result1;
        double result2;
        if (Math.pow(b, 2) - (4 * a * c) <= 0)
            return "These numbers do not compute - they produce an illegal result.";
        result1 = (-b + Math.sqrt(Math.pow(b, 2) - (4 * a * c))) / (2 * a);
        result2 = (-b - Math.sqrt(Math.pow(b, 2) - (4 * a * c))) / (2 * a);
        return String.valueOf(result1) + ", " + String.valueOf(result2);
    }
}

Among the things I really dislike are:

The static scanner object.
The three different checks for -1 being entered.
The quadratic formula method, which seems like it could be shortened.

I'm still working on this myself, but as I said any advice (including on naming conventions, code style etc.) would be appreciated.

A few things jump me here:

1. Class Names

Your class name is queer. The Java standard is to use PascalCasing for class names. You use Pascal_Snake_Case. That's kinda funny ;) Also you named your class after what it contains somewhere deep down, and not what it's responsible for. I'd instead probably name it Program. As it's nothing else, than that. I'd expect a class named  Quadratic_Equations to be an enum containing different Equations.

2. checkIfValidNumber

I like the name. It's good, it could describe what it does. I don't like the print-statement and that you have a loop in there. This is one of these "side-effects". Your function name makes me expect something different.
I expect a function applying some constraint to a number and returning true or false depending on the validity of that number to the constraint.

You also hide your whole process behind that. Also: why should a user not enter -1.0 for your quadratic equation? It makes no sense to use a number within the valid range to check for validity. instead use boolean return type in your tryParse and make use of the out-parameter.

3. Reading user input.

double a = tryParse(sc.nextLine());
  a = checkValidNumber(a);

This makes no sense. You should also move the first tryParse(sc.nextLine()); into a method.

Instead I would expect:

double a = promptUserUntilValidInput("Please enter a:");

You can move the whole reading to this method.

private double promptUserUntilValidInput(String prompt){
    boolean valid = false;
    double value = 0.0;
    while(!valid){
       System.out.println(prompt);
       //this is the c# attempt...
       valid = tryParse(sc.nextLine(), out value);

       //You would need to do something like this in java
       Double result = tryParse(sc.nextLine());
       valid = result != null;
       value = (double)result;
    }
    return value;
}

tryParse would then (in C#) return true, when parsing the number was successful. You'd just need to assign to an out parameter before returning in the method:

private boolean tryParse(String text, out double val){
     try{
        val = new Double(text);
        return true;
     }
     catch (NumberFormatException e){
        return false;
     }
}

This way you also eliminate all checks for -1.0.

But as this is java we need to make it a little more complicated. I'm here heavily relying on @Uri Agassi's answer

private Double tryParse(String text){
    try{
       Double value = new Double(text);
       return value;
    }
    catch (NumberFormatException e){
       if(text.matches("/^quit$/gi"))
           Application.Exit;
       else
         return null;
     }
}


Get XML elements with a particular attribute value I have the following code to find xml elements that have a particular attribute value:

IEnumerable<XElement> elems =
    xmlDoc.Descendants("elemName")
    .Where(x =>
        x.Attribute("attrName").Value
        .Equals(
            filename,
            StringComparison.OrdinalIgnoreCase
        )
    );

The problem is that if the element does not have that attribute, according to the docs, it will return null and we'll get a crash when we try to do null.Value.

I've rewritten it like this:

IEnumerable<XElement> elems =
     xmlDoc.Descendants("elemName")
     .Where(x =>
         ((x.Attribute("attrName") == null)
         ? ""
         : x.Attribute("attrName").Value).Equals(
             filename,
             StringComparison.OrdinalIgnoreCase
         )
     );

But I think it's a bit ugly. Is there an idiomatic way of doing this - I imagine this is a fairly common operation? I suppose I could just do a regular loop.

I think you could use the null-conditional operator for x.Attribute("attrName").Value and swap arguments of String.Equals to avoid the NullReferenceException as follows:

IEnumerable<XElement> elems =
     xmlDoc.Descendants("elemName")
     .Where(x =>
         filename.Equals(
             x.Attribute("attrName")?.Value,
             StringComparison.OrdinalIgnoreCase
         )
     );


Validating user supplied input I was assigned a task to fix the SQL injection flaw reported by a code analysis tool. I am not the original author of the code. I am fairly knowledgeable with SQL.

 public int ExecuteNonQuery(string query, SqlParameter[] parameters)
 {
     using (SqlCommand command = CreateCommand(query, parameters))
     {
         int rowsAffected = command.ExecuteNonQuery();
         log.Debug("rows affected: {0}", rowsAffected);
         return rowsAffected;
     }
 }

and the tool has reported the injection flaw at the line number 3. The ExecuteNonQuery is being called from so many places.

Here is one such sample:

public void AddDataFrom(IWorkflowStepExecutionStorage storage, bool inverseSourceAndTargetSide)
{
    WorkflowStepExecutionStorage storageImpl = storage as WorkflowStepExecutionStorage;
    if (storageImpl == null)
    {
        throw new InvalidOperationException();
    }
    const string queryTemplate =
        @"insert into [$mappingDictionaryTable](sourceObjectId, sourceObject, targetObjectId, targetObject, linkedObjectTypesPairId)
          select sourceTable.objectId, sourceTable.systemEntry, targetTable.objectId, targetTable.systemEntry, @linkedObjectTypesPairId from
            [$mappingTable] mt
            inner join [$sourceImportStorageTable] sourceTable on mt.$sourceIdColumn = sourceTable.id
            inner join [$targetImportStorageTable] targetTable on mt.$targetIdColumn = targetTable.id
        ";
    QueryBuilder queryBuilder = new QueryBuilder(queryTemplate);
    queryBuilder.AddParameter("$mappingDictionaryTable", this.mappingDictionaryTable.TableName);
    queryBuilder.AddParameter("$mappingTable", storageImpl.MappedTable.TableName);
    queryBuilder.AddParameter("$sourceImportStorageTable", inverseSourceAndTargetSide ?
        (storageImpl.TargetImportStorage as ImportExecutionStorage).ImportTable.TableName :
        (storageImpl.SourceImportStorage as ImportExecutionStorage).ImportTable.TableName);
    queryBuilder.AddParameter("$targetImportStorageTable", inverseSourceAndTargetSide ?
        (storageImpl.SourceImportStorage as ImportExecutionStorage).ImportTable.TableName :
        (storageImpl.TargetImportStorage as ImportExecutionStorage).ImportTable.TableName);
    queryBuilder.AddParameter("$sourceIdColumn", inverseSourceAndTargetSide ? MappedTable.TargetIdColumn.Name : MappedTable.SourceIdColumn.Name);
    queryBuilder.AddParameter("$targetIdColumn", inverseSourceAndTargetSide ? MappedTable.SourceIdColumn.Name : MappedTable.TargetIdColumn.Name);

    string query = queryBuilder.GetParametrizedQuery();
    SqlParameter[] sqlParams = new[] { new SqlParameter("@linkedObjectTypesPairId", storageImpl.RunConfiguration.LinkedTypesPairConfiguration.Id) };

    using (IQcDbConnection dbConnection = this.connectionProvider.CreateDbConnection())
    {
        dbConnection.ExecuteNonQuery(query, sqlParams);
    }
}

Here is the CreateCommand code:

private SqlCommand CreateCommand(string commandText, SqlParameter[] parameters)
 {
     SqlCommand retVal = this.connection.CreateCommand();
     retVal.CommandText = commandText;
     retVal.CommandTimeout = this.commandsTimeout;
     retVal.Parameters.AddRange(parameters);
     return retVal;
}

Is there any injection risk in this code? If yes, how do I recognize it?

There's only so much a code analysis tool can see.

using (SqlCommand command = CreateCommand(query, parameters))

You're using a parameterized SqlCommand, and given the example you appear to have everything under your own control, strongly typed and all.

Depends who is calling this ExecuteNonQuery method, and where query is coming from.

It appears the query is coming from a QueryBuilder, out of a QueryTemplate.

The thing is, this QueryBuilder has been given the ability to perform replacement of "tokens" in the SQL command string, and it seems to be taking in a string for the replacement - if I were a code analysis tool, I'd raise a flag right there.

I don't see any apparent specific problem with the example usage you've supplied. But how the API is used is the responsibility of the programmer using it, and the API takes a SQL command string, and actually sends another to the server. The API could easily be misused to misplace these "tokens" and pass a user-supplied value as the replacement value, and bingo, you have SQL injection happening.

In other words, I think there is a potential SQL injection issue with the API, if it's misused. But if it's used everywhere the way you've shown, in my opinion you can put that warning onto the "false positives" pile.

Using an Object Relational Mapper, or any current technology for that matter (Entity Framework comes to mind), could significantly reduce all that boilerplate - and risk of misuse. But no API is shielded from carelessness.

I can't help but mention this one:

SqlCommand retVal = this.connection.CreateCommand();

retVal reads like VBA code from 1998. You're using readable identifiers everywhere else; would returnValue kill anyone? Personally I use result all the time. I can live with params, but seeing retVal in my code base would automatically trigger Ctrl+R,R and a new name.


Dijkstra's algorithm in graph (Python) I need some help with the graph and Dijkstra's algorithm in Python 3. I tested this code (look below) at one site and it says to me that the code took too long. Can anybody suggest how to solve that? I don't know how to speed up this code. I read many sites but I didn't find normal examples...
P.S. this is a new code version with deque and something like this, but it's still too slow.
from collections import deque

class node:

    def __init__(self, name, neighbors, distance, visited):
        self.neighbors = neighbors
        self.distance = distance
        self.visited = visited
        self.name = name

    def addNeighbor(self, neighbor_name, dist): # adding new neighbor and length to him

        if neighbor_name not in self.neighbors:
            self.neighbors.append(neighbor_name)
            self.distance.append(dist)

class graph:

    def __init__(self):
        self.graphStructure = {} # vocabulary with information in format: node_name, [neighbors], [length to every neighbor], visited_status

    def addNode(self, index): # adding new node to graph structure

        if self.graphStructure.get(index) is None:
            self.graphStructure[index] = node(index, [], [], False)

    def addConnection(self, node0_name, node1_name, length): # adding connection between 2 nodes
        n0 = self.graphStructure.get(node0_name)

        n0.addNeighbor(node1_name, length)

        n1 = self.graphStructure.get(node1_name)
        n1.addNeighbor(node0_name, length)

    def returnGraph(self): # printing graph nodes and connections

        print('')
        for i in range(len(self.graphStructure)):
            nodeInfo = self.graphStructure.get(i + 1)
            print('name =', nodeInfo.name, '  neighborns =', nodeInfo.neighbors, '           length to neighborns =', nodeInfo.distance)

    def bfs(self, index): # bfs method of searching (also used Dijkstra's algorithm)

        distanceToNodes = [float('inf')] * len(self.graphStructure)
        distanceToNodes[index - 1] = 0
        currentNode = self.graphStructure.get(index)
        queue = deque()

        for i in range(len(currentNode.neighbors)):
            n = currentNode.neighbors[i]
            distanceToNodes[n - 1] = currentNode.distance[i]
            queue.append(n)

        while len(queue) > 0: # creating queue and visition all nodes
            u = queue.popleft()
            node_u = self.graphStructure.get(u)
            node_u.visited = True

            for v in range(len(node_u.neighbors)):
                node_v = self.graphStructure.get(node_u.neighbors[v])
                distanceToNodes[node_u.neighbors[v] - 1] =  min(distanceToNodes[node_u.neighbors[v] - 1], distanceToNodes[u - 1] + node_u.distance[v]) # update minimal length to node
                if not node_v.visited:
                    queue.append(node_u.neighbors[v])

        return distanceToNodes

def readInputToGraph(graph): # reading input data and write to graph datatbase
    node0, node1, length = map(int, input().split())

    graph.addNode(node0)
    graph.addNode(node1)
    graph.addConnection(node0, node1, length)

def main():
  newGraph = graph()
  countOfNodes, countOfPairs = map(int, input().split())

  if countOfPairs == 0:
      print('0')
      exit()

  for _ in range(countOfPairs): # reading input data for n(countOfPairs) rows
      readInputToGraph(newGraph)

  # newGraph.returnGraph() # printing information

  print(sum(newGraph.bfs(1))) # starting bfs from start position

main()

The input graph structure may look like this:
15 17
3 7 2
7 5 1
7 11 5
11 5 1
11 1 2
1 12 1
1 13 3
12 10 1
12 4 3
12 15 1
12 13 4
1 2 1
2 8 2
8 14 1
14 6 3
6 9 1
13 9 2

I'm only learning python so I think I may be doing something wrong.

Making an str(which makes a string from an object) for the node class which would simplify the graph return method
the str converts an object to a string type

is this a dense graph or sparse graph?
Sparse Graphs have a lot less edges between vertices
Sparse Graphs can be efficently used as adjency lists
Dense Graphs can be efficnetly represented with an adjency matrix (2d list of bool)
Note: if you are familar with binary, you can use an int to set and get bits in Python (more space efficent than 2d list)

I would suggest a new class for what to do when the algorithm reaches a node (such as NodeVisit)
This new class can a function called call(node) and run as object(the_node)
This allows diffrent code to run on node such as print out or add distances along a path

Dijkstra's algorithm has a lower Big O with a min priority que
Min priority ques are similar to real life lines with priority levels and tasks
This can be made from a list where left_row is 2 * row and right_row is (2 * row) + 1

Make a seperate function or even class for Dijkstra's algorithm
Dijkstra's algorithm can be done using parallel lists for vertex, distance, prev vertex
Looking up the wikipedia article for Dijkstra's Algorithm shows its psudo code

If you plan to use node anywhere else then make self.visited a parallel array and not inside the node class
Another situation where int and binary can be used for space efficency


Mirrors number at borders of interval, untill it lays in the interval I have the following simple function:

__device__
mirror(int index , int lB, int uB)
{
    while(index < lB || index >= uB)
    {
        if(index < lB) {
            index = lB + (lB-index);
        }

        if(index >= uB) {
            index = uB-1 -(index-uB);
        }
    }
    return index;
}

Assume lb <= uB.

It is used for every index to give it a mirror border behaviour and make sure the result is a valid index.

Are there ways to improve this, with or without changing the result for indices outside [lB-d,uB+d), where d >= uB-lB.

mirror(i,lB,uB) has to be in [lB,uB) of course.

What are concerns for performance?

this function is called very often
it is called from an kernel (cuda things relevant? thread divergence?)

Are there possible improvments for the special case lB == 0?

EDIT: added this small test example

__global__
void driver(unsigned char* img_dest, unsigned char* img_src, int width, int height)
{
    int x = blockIdx.x*blockDim.x+threadIdx.x;
    int y = blockIdx.y*blockDim.y+threadIdx.y;
    if(x >= 0 && x < width && y >= 0 && y < height)
    {
        int x_src = mirror(x*2-width/2,0,width);
        assert(x_src >= 0);
        assert(x_src < width);
        int y_src = mirror(y+x,0,height);
        assert(y_src >= 0);
        assert(y_src < height);
        img_dest[x+y*width] = img_src[x_src + y_src*width];
    }
}

// let the images be gray images, with no pitch.
void callDriver(unsigned char* img_dest, unsigned char* img_src, int width,int height)
{
    dim3 block(16,16);
    dim3 grid((width+block.x-1)/block.x,(height+block.y-1)/block.y);
    driver<<<grid,block>>>(img_dest,img_src,width,height);
    cudaDeviceSynchronize();
}

main()
{
    int width = 512;
    int height = 256;
    unsigned char* img_dest;
    cudaMalloc(&img_dest,width*height);
    unsigned char* img_src;
    cudaMalloc(&img_src,width*height);
    callDriver(img_dest,img_src,width,height);
}

My use case is similar to the driver example:
I have functions deforming where I would have to load pixel that would lay outside of the defined region. To provide meaningful values I want to define them by mirroring at the borders.

My use case are distortion, and linear filters. In both cases I have to use values for pixels that are not defined in the image. While in the case of linear filters, there are other possible solutions (starting extra kernel for border), I decided to work with one kernel for this time.

__device__
mirror(int index , int lB, int uB)

You've got an extra space before the first comma, and I think you're missing the function's return type entirely. You should always compile your code with -Wall and fix all the warnings before posting it (or before running it). There's no point in shipping code with bugs.

Similarly funky whitespace on this line:

            index = uB-1 -(index-uB);

Anyway, if you're looking to speed up a piece of code with loops in it, your first thought should be, "How do I get rid of these loops?"

Consider that after you "mirror" index off of both boundaries, you'll simply have reduced its overall value by 2*(uB - lB). Proof:

index = uB-1 - (index-uB);
index = lB + (lB-index);

means

index = lB + lB - (uB - 1 - index + uB);

means

index = 2*lB - 2*uB + 1 + index;

means

index = index - (2*(uB - lB) - 1);

Hmm... I see you have an extra -1 in there. Was that intentional? Let's see if we can reproduce it in a test case.

assert(mirror(0, 0, 3) == 0);
assert(mirror(1, 0, 3) == 1);
assert(mirror(2, 0, 3) == 2);
assert(mirror(3, 0, 3) == 2);
assert(mirror(4, 0, 3) == 1);
assert(mirror(5, 0, 3) == 0);
assert(mirror(6, 0, 3) == 1);
assert(mirror(7, 0, 3) == 2);
assert(mirror(8, 0, 3) == 2);

Yep, the two boundaries behave differently! The moral of the story is: Always test your code.

Now that we know how the code behaves (which, honestly, probably isn't how you intended it to behave) — we can modify it safe in the knowledge that we won't introduce bugs. We just have to keep all our test cases passing. (Of course we should write some test cases with negative inputs, too.)

Any time you have code with repeated addition, you should think about whether it can be replaced with multiplication; and any time you have repeated subtraction, you should think about whether it can be replaced with division (which is to say, modulus).

int mirror(int index, int lB, int uB) {
    int n = uB - lB;
    int period = 2*n - 1;  // the pattern repeats with this period
    int mod_p = (index - lB) % period;
    if (mod_p < n) {
        return lB + mod_p;
    } else {
        return uB - 1 - (mod_p - n);
    }
}


Copying from a directory without using os.chdir() The program should backup files in a source folder to a zip file in destination, and keep track of how many versions there are (which explains the shelve). In order to copy these files, apparently I have to change the cwd to the source path (folder), because otherwise the program will not detect them. While it may not seem to cause that much of an issue, my OCD triggers when I see the shelve files in the new .zip folder. I thought about adding lines to delete the shelve files, but I was wondering if there is some other way.

(This program is a project in a certain book I'm reading, and while the project demanded a more basic version of this, I like to take it a step forward.)

import os, shelve, zipfile

def backupToZip(folder, destination, copies=1):

#Folder
    fName= os.path.basename(folder)

#Shelfing for copy number
    namesNums = shelve.open('names')
    if fName not in namesNums:
        num= 1
        namesNums[fName]= num
    else:
        num= namesNums[fName]

#Producing zips
    os.chdir(folder)
    for i in range(int(copies)):
        zipPath= os.path.join(destination, fName+'_'+str(num)+'.zip')
        theZip= zipfile.ZipFile(zipPath, 'w')

        for foldername, subfolders, filenames in os.walk(folder):
            for filename in filenames:
                theZip.write(filename)
        num+=1

    namesNums[fName]= num
    namesNums.close()

shelve supports all methods that dictionarys do.

  Shelf objects support all methods supported by dictionaries. This eases the transition from dictionary based scripts to those requiring persistent storage.

And so you can use dict.setdefault, rather than your if-else.
Rather than using num += 1, you can use range(num, num + copies). Since num is defined in an outer scope, you can use it after the for loop too.
Rather than using string concatination, you can use f-strings in Python 3.6+ or format strings:

# yours
fName+'_'+str(num)+'.zip'
# f-string
f'{fName}_{num}.zip'
# string format
'{}_{}.zip'.format(fName, num)
'{fName}_{num}.zip'.format(fName=fName, num=num)

It's simpler to use with when handling content managers. And so you should use them on shelve.open and zipfile.ZipFile.

Not only does it make you not have to learn how to close all types of context managers. The context will be closed in the case of an error too.
Don't use CamelCase or camelCase, instead use snake_case.
Use _ as a 'throw away' variable. Some people, however, like to instead use __.
Put a space either side of your equal signs. a = b, not a= b.

This can become:

import os, shelve, zipfile

def backup_to_zip(folder, destination, copies=1):
    f_name= os.path.basename(folder)
    with shelve.open('names') as names:
        num = names.setdafault(f_name, 1)

        os.chdir(folder)
        for num in range(num, num + copies):
            zip_path = os.path.join(destination, f'{f_name}_{num}.zip')
            with zipfile.ZipFile(zip_path, 'w') as the_zip:
                for _, _, filenames in os.walk(folder):
                    for filename in filenames:
                        the_zip.write(filename)

        names[f_name] = num

You could also use pathlib rather than os.path. Which may allow you to use:

def backup_to_zip(folder, destination, copies=1):
    folder = Path(folder)

    with shelve.open('names') as names:
        num = names.setdefault(folder.name, 1)
        path = folder / Path(destination)
        for num in range(num, num + copies):
            with zipfile.ZipFile(path / f'{folder.name}_{num}.zip', 'w') as f:
                for file in folder.iterdir():
                    if file.is_file():
                        f.write(file.name)

        names[folder.name] = num


How is OOP achieved with configuration files in PHP? Someone asked if their singleton is good for OOP:

Using a singleton class to get and set program wide settings.

However, the answer just provides tweaks to his existing code - but the question wasn't entirely answered. What is the best practice to use for configuration files so that it doesn't become spaghetti code?

All apps need configuration settings so that it saves time and it looks neat.

I currently use something similar to what was proposed but not a singleton.  However, I have a long array that feeds into different constructors. I feel that this is improper or maybe it is.

namespace App;

class Config
{
    protected static $config = array();

    public static function get($name, $default = null)
    {
        return isset(self::$config[$name]) ? self::$config[$name] : $default;
    }
    public static function add($parameters = array())
    {
        self::$config = array_merge(self::$config, $parameters);
    }
}
App\Config::add(
    array(
        'database' => array(
            'default' => array(
                'driver'    => 'mysql',
                'host'      => '127.0.0.1',
                'username'  => 'user',
                'password'  => '',
                'database'  => 'database',
                'charset'   => 'utf8',
                'collation' => 'utf8_unicode_ci'
            ),
            'tracking' => array(
                'driver'    => 'mysql',
                'host'      => '192.168.0.1',
                'username'  => 'tracking',
                'password'  => '123456',
                'database'  => 'tracking',
                'charset'   => 'utf8',
                'collation' => 'utf8_unicode_ci'
            ),
            'data' => array(
                'driver'    => 'mysql',
                'host'      => '192.168.0.11',
                'username'  => 'data',
                'password'  => 'abcder',
                'database'  => 'data_info',
                'charset'   => 'utf8',
                'collation' => 'utf8_unicode_ci'
            )
        ),

        'pheanstalk' => array(
            'default' => array(
                'host'       => 'localhost',
                'port'       => 11400,
                'queue_name' => 'queue'
            )
        ),
        'app_worker' => array(
            'log_files_path'      => ROOT.'/logs',
            'file_size_limit'     => 542880, //5MB
            'file_name_extension' => 'w_log',
        ),
        'memcache' => array(
            'default' => array(
                array(
                    'host'  => 'localhost',
                    'port'  => '11211'
                ),
                array(
                    'host'  => 'localhost',
                    'port'  => '11211'
                ),
                array(
                    'host'  => 'localhost',
                    'port'  => '11211'
                )
            )
        )
    )
);

Then in my main program:

$db_config = App\Config::get('database');

Now it's ok for me to add extra configuration needs for different object I might create and etc and I can load up a controller or piece of code to fetch it from.  However, I feel that this configuration file that adds all these settings will be terribly long in the long wrong.

I would like to know if there something better, and what. I tried to use inheritance or DI but my configuration file is still long.

I personally use defines for for settings rather than utilising arrays.

A singleton is the correct route to go down and to help reduce your code and array you may want to use external files.

This is my config loader class with some simple caching that supports both database and file;

<?php
/**
 * Config loader
 */
final class Config extends Singleton{
    public static $loadedConfigs = array();

    /**
     * Get specific config value
     *
     * @param String $scope  Configuration scope, used to determine config location
     * @param String $module Specific config module name
     * @param String $param  (optional) Specific config option
     *
     * @return Mixed    Requested parameter value if requested, otherwise associative array or configuration key:pair values unless config file couldn't be loaded, then false
     * @author Dean Whitehouse
     * @copyright Dean Whitehouse 2014
     * @todo verify the config name is a valid one
     */
    public static function get($scope, $module, $param = null){
        if($param !== null && $param != ''){
            $const = strtoupper((string)$param);
        }

        $errorAppend = '; requested scope `'.$scope.'` with module `'.$module.'`;'.($param !== null ? 'Parameter requested `'.$param.'`' : '');

        if(isset(static::$loadedConfigs[$scope][$module])){
            $config = static::$loadedConfigs[$scope][$module];

            if(isset($const)){
                if(!isset($config[$const])){
                    error_log('Constant `'.$const.'` undefined'.$errorAppend);
                    return false;
                }
                else
                    $config = $config[$const];
            }
            return $config;
        }
        else{
            if($scope == 'core'){
                $file = '_app/core/config/'.$module.'.config.php';

                if(!file_exists($file)){
                    error_log('Config file doesn\'t exist at `'.$file.'`'.$errorAppend);
                    return false;
                }
                else{
                    $consts = get_defined_constants(true);
                    $consts = isset($consts['user']) ? $consts['user'] : array();

                    require_once $file;

                    $new = get_defined_constants(true);
                    $new = $new['user'];
                    $diff = array_diff_assoc($new, $consts);
                    static::$loadedConfigs[$scope][$module] = $diff;

                    if(isset($const)){
                        if(defined($const))
                            return constant($const);
                        else{
                            error_log('Constant `'.$const.'` undefined'.$errorAppend);
                            return false;
                        }
                    }

                    return static::$loadedConfigs[$scope][$module];
                }
            }
            elseif($scope == 'coreDB'){
                $check = db\sqli::query("SHOW TABLES LIKE '".db\sqli::escape($module)."'; -- Database check");

                if($check->num_rows == 0){
                    error_log('Config table doesn\'t exist'.$errorAppend);
                    return false;
                }
                else{
                    $query = 'SELECT `key`, `value` FROM `'.db\sqli::escape($module).'`';
                    if($param !== null){
                        $query .= ' WHERE `key` = '.db\sqli::escape($param).' LIMIT 1';
                    }

                    $query = db\sqli::query($query);

                    if($query->num_rows > 0){

                        $consts = get_defined_constants(true);
                        $consts = isset($consts['user']) ? $consts['user'] : array();

                        while($config = $query->fetch_assoc()){
                            define($config['key'], $config['value']);
                        }

                        $new = get_defined_constants(true);
                        $new = $new['user'];
                        $diff = array_diff_assoc($new, $consts);
                        static::$loadedConfigs[$scope][$module] = $diff;

                        if(isset($const)){
                            if(defined($const))
                                return constant($const);
                        }

                        return static::$loadedConfigs[$scope][$module];
                    }
                    else{
                        error_log('Constant `'.$const.'` undefined'.$errorAppend);
                        return false;
                    }

                }
            }
            else{
                error_log('Config scope `'.$scope.'` isn\'t recognised'.$errorAppend);
                return false;
            }
        }
    }

    /**
     * Load configuration module
     *
     * @param String $scope  Configuration scope, used to determine config location
     * @param String $module Specific config module name
     *
     * @return Mixed    False if configuration couldn't be loaded, otherwise associative array or configuration key:pair values
     * @author Dean Whitehouse
     * @copyright Dean Whitehouse 2014
     */
    public static function load($scope, $module){
        return static::get($scope, $module);
    }
}
?>

I call it like:

Config::load('core', 'env');
Config::load('core', 'db');

Edit

Sample config file 'env.config.php'

<?php
define('AUTO_REPAIR', true);

define('APP_ROOT', rtrim($_SERVER['SCRIPT_NAME'], '/index.php')); //folder the root of the site is in - only if in a directory

define('SECURE', true);

define('LOG_DIR', '_app/logs/');

define('SECURE_KEY', ''); //used for CSRF, NONCE and Cookies
?>


Insert elements into a map container I am inserting elements into the map container. For this I am using the same statements multiple times in the below function. Is there any way I can write a generic function for this? So that even at the later point of time, when required, I can insert new elements.

 bool EMRMgr::GetParams()
    {
        EmIOStruct emIOstructObj;
        WCHAR szValue[MAX_PATH] = { 0 };
        DWORD dwDataSize = sizeof(szValue) / sizeof(WCHAR);
        long lRes = 0;
        McStorageType mcStorageTypeObj = McStorageType::eRegistry;
        std::wstring value;

    // First time I am using the below statements to insert elements into the map container

        mcIOstructObj.lpszValueName = (LPWSTR)ER_ID;
        memset(szValue, 0, MAX_PATH);
        mcIOstructObj.lpData = (LPBYTE)&szValue[0];
        lRes = m_cIOManager.ReadValue(mcStorageTypeObj, mcIOstructObj);
        value.clear();
        if ((LPWSTR)mcIOstructObj.lpData == nullptr)
        {
            value.assign(L"");
        }
        else
        {
            value.assign((LPWSTR)mcIOstructObj.lpData);
        }

        m_fileParams.insert({ (std::wstring) ER_ID, value });

    // Second time I am using the below statements to insert elements into the map container

        mcIOstructObj.lpszValueName = (LPWSTR)CPS;
        memset(szValue, 0, MAX_PATH);
        mcIOstructObj.lpData = (LPBYTE)&szValue[0];
        lRes = m_cIOManager.ReadValue(mcStorageTypeObj, mcIOstructObj);
        value.clear();
        if ((LPWSTR)mcIOstructObj.lpData == nullptr)
        {
            value.assign(L"");
        }
        else
        {
            value.assign((LPWSTR)mcIOstructObj.lpData);
        }
        m_fileParams.insert({ (std::wstring) CPS, value });

        return true;
    }

There is missing code so I'm assuming that ER_ID and CPS are strings. If not, you can easily sub the data type.

Since the only difference between the blocks of code is which string (or other data type) to use (ER_ID vs. CPS), you can make a member function of EMRMgr that has the same block of code but subbing the string to use with a parameter.

Unless I've overlooked something, this should work.

bool EMRMgr::DoTheThing(std::string str)
    {
        mcIOstructObj.lpszValueName = (LPWSTR)str;
        memset(szValue, 0, MAX_PATH);
        mcIOstructObj.lpData = (LPBYTE)&szValue[0];
        lRes = m_cIOManager.ReadValue(mcStorageTypeObj, mcIOstructObj);
        value.clear();
        if ((LPWSTR)mcIOstructObj.lpData == nullptr)
        {
            value.assign(L"");
        }
        else
        {
            value.assign((LPWSTR)mcIOstructObj.lpData);
        }

        m_fileParams.insert({ (std::wstring) str, value });

    }


Is there any flaw in the logic of this problem? The problem is find the number of factors of (N!)^2.

This is the code, I wrote:

int prime[]={//list of primes <(10^6)};
int num_fac_of_factorial(int n, int x)
{
    int p=x, z=0;
    while(n/p>0)
    {
        z+=n/p;
        p*=x;
    }
    return z;
}

int count=78498;   //Count of prime numbers <10^6

int main()
{
    int n, x, first=0, m=1000007;
    long long sol=1;
    scanf("%d", &n);

    for(int i=0; prime[i]<=n && i<count; i++)
    {
        x=num_fac_of_factorial(n, prime[i]);
        x=2*x+1;
        sol=(sol*x)%m;
    }
    sol%=m;
    printf("%d\n", sol);
}

There's no flaw in the logic, just a couple of implementation errors. And if e.g. 1/10 + 1/15 = 1/6 and 1/15 + 1/10 = 1/6 shall not be counted as different solutions but as one, then of course you need (1 + number of divisors)/2.

Here

for(int i=0; prime[i]<=n && i<count; i++)

you ought to check i < count before accessing prime[i]. If n > 999983, you will try to access prime[78498], which is undefined behaviour.

And - assuming that int is as usual 32 bits (or 36, wouldn't apply to 64-bit ints, however) - here

int num_fac_of_factorial(int n, int x)
{
    int p=x, z=0;
    while(n/p>0)
    {
        z+=n/p;
        p*=x;
    }
    return z;
}

you have overflow for large enough primes x (typically > 46340) when calculating x*x, which is also undefined behaviour. And with the not uncommon wrap-around for int overflow, that will cause wrong results for n >= 65537. Replace the while loop with

do {
    n /= x;
    z += n;
while(n > 0);

to avoid the overflow.


Finding the common prefix in a list of strings Given a list of strings, my task is to find the common prefix.

Sample input: ["madam", "mad", "mast"]
Sample output: "ma"

Sample input: ["question", "method"]
Sample output: ""

Below is my solution, I'd be happy for help improving the algorithm (I'm open to totally different approaches) or general code improvement tips.
Thanks :)

public class PreFixer {

    public static void main(String[] args) {
        if(args.length < 1) {
            System.out.println("invalid arguments");
            return;
        }
        String commongPrefix = getCommonPrefix(args);
        System.out.println("Common Prefix for list is : " + commongPrefix);
    }

    private static String getCommonPrefix (String[] list){
        int matchIndex = recursiveChecker(0, list);
        return list[0].substring(0, matchIndex);
    }

    private static int recursiveChecker(int strIndex, String[] list){

        for(int x=0; x<list.length; x++) {
            if(strIndex >= list[x].length()){
                return strIndex;
            }

            if(list[0].charAt(strIndex) != list[x].charAt(strIndex)) {
                return strIndex;
            }
        }

        return recursiveChecker(strIndex + 1, list);
    }
}

There are some inconsistencies in your code style: sometimes you do put a space before an opening curly bracket, sometimes you don't. It is a good practice to adhere to one style(in this case, it is conventional to have a whitespace there). It is also conventional to surround all binary operators with whitespaces to make the code more readable. For instance, for(int x = 0; x < list.length; x++) looks better than for(int x=0; x<list.length; x++).
In terms of time complexity, you algorithm is optimal(it is linear in the size of input). However, if it is supposed to work with long strings, I'd use iteration instead of recursion(it gets a StackOverflowError when the strings get really big). Here is my iterative solution:

public static String getLongestCommonPrefix(String[] strings) {
    int commonPrefixLength = 0;
    while (allCharactersAreSame(strings, commonPrefixLength)) {
        commonPrefixLength++;
    }
    return strings[0].substring(0, commonPrefixLength);
}

private static boolean allCharactersAreSame(String[] strings, int pos) {
    String first = strings[0];
    for (String curString : strings) {
        if (curString.length() <= pos
                || curString.charAt(pos) != first.charAt(pos)) {
            return false;
        }
    }
    return true;
}

In general, each class should have single responsibility(that it, you might create two separate classes here: one for computing the longest prefix and the other one for checking and parsing command-line arguments).  But I think it is fine to have one class here(the entire class is pretty small) as long as the format of arguments is not going to change in the future.


Java background server class that might be called from ui thread I have a server communicating over network interface. This server should run on a background thread, so that it does not block the ui thread. The ui thread starts and stops the server. Even starting and stopping should be done from a background thread.

Currently I wrap all public methods of the server class in a thread that gets started immediately:

class Server {

  public void start() {
    new Thread() {
      public void run() {
        handleStart();
      }
    }.start();
  }

  public void stop() {
    new Thread() {
      public void run() {
        handleStop();
      }
    }.start();
  }

  private void handleStart() {
    startLooperThread();
  }

  private void handleStop() {
    stopLooperThread();
  }
}

Is this the proper way to do this or are there any neat alternatives provided by JDK (or any other library)?

Disclaimer: This answer uses the lambda-feature for java 8. Assuming non-availability of java 8, the expressions can be easily converted to Runnables.

You're completely overexerting yourself. Use inbuilt features of Java with a little composition and your server becomes by far easier:

public class Server {
     private final ExecutorService threadPool = Executors.newFixedThreadPool(1);

     public Server() {}

     public void start() {
         threadPool.execute(() -> handleStart()); //Runnable
     }

     public void stop() {
         threadPool.execute(() -> handleStop());
         threadPool.shutdown(); //depends a little on your functionality
     }
     //rest can remain unchanged
}

The Thread-Pool can be expanded manually. Additionally I'd recommend to override finalize() to shut down your executor.


0-1 Knapsack in Java I'm looking for advice on coding practices in general.

 public class Knapsack {

        public static int getMaxBenefit(int[] weights, int[] benefits, int capacity) {

        int[][] maxbenefit = new int[weights.length + 1][capacity + 1];

        for (int i = 0; i <= weights.length; i++)
            maxbenefit[i][0] = 0;
        for (int i = 0; i <= capacity; i++)
            maxbenefit[0][i] = 0;

        for (int item = 1; item <= weights.length; item++) {

            for (int weight = 1; weight <= capacity; weight++) {

                if (weights[item - 1] <= weight) {

                    int option1 = benefits[item - 1] + maxbenefit[item - 1][weight - weights[item - 1]];
                    int option2 = maxbenefit[item - 1][weight];
                    maxbenefit[item][weight] = Math.max(option1, option2);

                } else {
                    maxbenefit[item][weight] = maxbenefit[item - 1][weight];
                }

            }

            // printMatrix(maxbenefit, weights.length, capacity);
        }

        printMatrix(maxbenefit, weights.length, capacity);

        return maxbenefit[weights.length][capacity];
    }

    public static void printMatrix(int[][] matrix, int R, int C) {
        for (int i = 0; i <= R; i++) {
            for (int j = 0; j <= C; j++) {

                System.out.print(matrix[i][j] + " | ");

            }

            System.out.println();
        }

    }

    public static void main(String[] args) {
        int[] weights = new int[] { 2, 3, 4, 5, 6 };
        int[] benefit = new int[] { 3, 4, 8, 9, 9 };
        getMaxBenefit(weights, benefit, 5);
    }

}

int[][] maxbenefit = new int[weights.length + 1][capacity + 1];

    for (int i = 0; i <= weights.length; i++)
        maxbenefit[i][0] = 0;
    for (int i = 0; i <= capacity; i++)
        maxbenefit[0][i] = 0;

There is no need to initialize part of the array as 0, because the entire array starts off as 0.

public static void printMatrix(int[][] matrix, int R, int C) {
    for (int i = 0; i <= R; i++) {
        for (int j = 0; j <= C; j++) {

            System.out.print(matrix[i][j] + " | ");

        }

        System.out.println();
    }

}

In here, you should check if R and C don't exceed matrix.length and matrix[i].length respectively. That way you can either limit the printing, or you could throw an error with explanation, rather than running into ArrayIndexOutOfBoundsException.

In the same way, you should check the size of int[] benefits in getMaxBenefit, because if the weights and the benefits are not the same size then you'll throw an error at some point, and failing fast is better.


Is my code fine in the areas of read and write? What:

This is a code that when executed simply writes packet to certain file types. Packet is very simple and contains things like:

description, partCount, price...

Please:

Ignore the usage of namespace std; as this is for educational
purposes, mostly. It will most likely not be reproduced in a real
program.
Focus on reviewing the read and write portions as I would love
feedback on those portions.
I used some other things for simplicity, but in general I am mainly
looking for feedback on read/write portions of the code.

Intent:

I'm specifically looking for feedback on the read and write portions of the code. Could I get feedback on it as well as is it feasible as a piece of code and what can I improve on/etc.???

Main.cpp:

#include <iostream>
#include <fstream>
#include <vector>

#include "Packet.h"

using namespace std;

int main() {
    cout << "----------------------------------------------------" << endl;
    cout << "Input and Output Filing System" << endl;
    cout << "----------------------------------------------------" << endl;
    ifstream inFile;
    ofstream outFile;
    fstream bOutFile;
    vector<Packet> package;
    Packet one(0, "A packet of cheese.", 21.99, 4);
    package.push_back(one);
    Packet two(1, "A packet of super-cheese.", 31.99, 2);
    package.push_back(two);

    cout << "Starting to write packets to file..." << endl;

    outFile.open("Insert Your Destination of File Here.Txt");
    for (int i = 0; i < package.size(); ++i) {
        outFile << "{" << endl;
        outFile << package[i].getPartId() << "," << endl;
        outFile << package[i].getDescription() << "," << endl;
        outFile << package[i].getPartCount() << "," << endl;
        outFile << package[i].getPrice() << "," << endl;
        outFile << "}" << endl << endl;
    }
    outFile.close();

    cout << "Starting to read packets from file..." << endl;

    inFile.open("Insert Your Destination of File Here.Txt");
    string line;
    while (getline(inFile, line)) {
        if (line[0] != '{' && line[0] != '}') {
            cout << line.substr(0, line.size() - 1) << endl;
        }
    }
    inFile.close();

    cout << "Starting to write packets to binary..." << endl;

    bOutFile.open("Insert Your Destination of File Here.bin", ios::out | ios::binary);
    for (int i = 0; i < package.size(); ++i) {
        bOutFile.write((char*)&package[i], sizeof(package[i]));
    }
    bOutFile.close();

    system("pause");
}

Packet.h:

#pragma once
#include <string>

using namespace std;

class Packet {
public:
    Packet(int partId, string description, double price, int partCount) :
        partId(partId), description(description), price(price), partCount(partCount) {}
    int getPartId() const { return partId; }
    string getDescription() const { return description; }
    double getPrice() const { return price; }
    int getPartCount() const { return partCount; }

private:
    int partId;
    string description;
    double price;
    int partCount;
};

for (int i = 0; i < package.size(); ++i)

I'd use for (auto& p : package) instead if the index (i) is not used for anything.

outFile << package[i].getPartId() << "," << endl;

Remember that std::endl flushes the stream. This is very minor, but I'd use a manual '\n' and then flush when the whole object is written.

bOutFile.write((char*)&package[i], sizeof(package[i]));

This is I do not like at all. Copying objects like this is problematic when you have objects with more complex data. I.e. pointers or dynamically allocated memory.

Take a look at serialization/deserialization, I found this read quite good concerning the topic: https://rubentorresbonet.wordpress.com/2014/08/25/an-overview-of-data-serialization-techniques-in-c/

Packet(int partId, string description, double price, int partCount

Make the passed arguments const references instead! (Otherwise an extra copy will be done)


Python Simple Range and Bearing Calculator using Complex Numbers I have just written this code that takes an initial coordinate and returns a new vector depending on a user-supplied range and bearing.
The calculation of the new position is done using Complex Numbers. I am sure there are easier ways to do this, but I wanted to play around with complex numbers. Any thoughts on these would be great.
The calculator is in a file called GenGeo, as this is where I keep lots of little modules like this that may be called again in other programs.
I have used **kwargs for a_e and a_n as these may not be required by the user. However, I have never really used *args and **kwargs before, so presume that I may have messed something up here.
This is the calculator module in GenGeo:
class CmO:
    """
    This class will contain specific code that is required to run a C-O calculation
    This includes range and bearing calc
    """

    def __init__(self, *args, **kwargs):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """
        self.a_e = kwargs.get('a_e', None)
        self.a_n = kwargs.get('a_n', None)

    def complex_randb(self, r, b):
        """
        An equation that using imaginary numbers to calculate the coordinates of a new
        point from a range and bearing of an old point
        :param r: range from original coordinate to new coordinate
        :param b: bearing from original coordinate to new coordinate
        """

        # -b is required as geodetic bearings are opposite to mathematical bearings
        t = complex(cos(radians(-b)), sin(radians(-b))) * complex(0, r)
        delta_easting = t.real
        delta_northing = t.imag

        if self.a_e and self.a_n is not False:

            new_easting = self.a_e + delta_easting
            new_northing = self.a_n + delta_northing

            return new_easting, new_northing

        else:
            return delta_easting, delta_northing

and this is the program that I have used to call it:
from GenGeo import CmO

initial_E = 100
initial_N = 100
input_range = 3
input_bearing = 310

a = CmO(a_e=initial_E, a_n=initial_N)
nE, nN = a.complex_randb(input_range, input_bearing)

print(f"The delta Easting and Northing are {round(nE, 3)}mE and {round(nN, 3)}mN")

Next project is writing something to calculate the range and bearing between two points
Any thoughts or improvements would be greatly appreciated!

Keyword-only parameters
You are not using **kwargs correctly.  **kwargs should be used when you can accept any number of keywords, but you don't know what the keywords will be, such as dictionary creation.  If you only accept 2 keyword parameters, you should list those keyword parameters explicitly:
    def __init__(self, *, a_e=None, a_n=None):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """

        self.a_e = a_e
        self.a_n = a_n

That * marks the end of positional parameters.  a_e and a_n can be specified by keyword only.  Since both default to None, both are optional.  Any other keywords is reject with an error message, instead of silently being ignored.
Operator Precedence
        if self.a_e and self.a_n is not False:

This statement does not do what you think it does.  is not is higher precedence than and, so it reads:
        if self.a_e and (self.a_n is not False):

thus, if a_n is never given as False, the result of is not will be True, and the and will always result in the truthiness of a_e only.
You probably intended the evaluation to be:
        if (self.a_e and self.a_n) is not False:

which tests the truthiness of both a_e and a_n.  Sort of.  There are very few ways of getting something for which is not False is not true out of that expression.  The only way to get to the else expression is if a_e == False, or if a_e held a truthy value and a_n == False.  Again, since if the values are not given, they are defaulted to None, and since None and None evaluates to None, and None is not False is a true statement, the if clause would be executed.
So you probably wanted to write:
        if self.a_e is not None and self.a_n is not None:

Why not Zero?
If you used 0 as the default for a_n and a_e, then
            new_easting = self.a_e + delta_easting
            new_northing = self.a_n + delta_northing

new_easting would simply become delta_easting and new_northing would become delta_northing, and you could always do the addition and return `new_easting, new_northing.
    def __init__(self, *, a_e=0, a_n=0):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """

        self.a_e = a_e
        self.a_n = a_n

    def complex_randb(self, r, b):
        """
        An equation that using imaginary numbers to calculate the coordinates of a new
        point from a range and bearing of an old point
        :param r: range from original coordinate to new coordinate
        :param b: bearing from original coordinate to new coordinate
        """

        # -b is required as geodetic bearings are opposite to mathematical bearings
        t = complex(cos(radians(-b)), sin(radians(-b))) * complex(0, r)
        delta_easting = t.real
        delta_northing = t.imag

        new_easting = self.a_e + delta_easting
        new_northing = self.a_n + delta_northing

        return new_easting, new_northing

Naming
Your parameter names a_e, a_n, r, and b are too short and cryptic.  You should used easting northing, range, and bearing.
complex_randb is also confusing.  The input is real, the output is real.  The fact that complex numbers are internally used is an internal detail irrelevant to the caller.  What is randb, some kind of random b?  Oh, range_and_bearing!  But it is not a range and bearing function, it is a new coordinate function:
    def new_coordinate(self, range, bearing):
        """Doc-string without the complex number internal detail mentioned"""
        ...

The class name CmO is also quite cryptic.  Is that "C minus O" because it runs "C-O" calculations?  You need a better class name.


Javascript deep proxy to detect changes I'm trying to build a deep proxy to observe all the changes in an object in javascript. Here is what I got so far :

function GetDeepProxy(value, config, path = 'root')
{
    var handler = function(config) {
        return {
            set: function(target, name, value) {
                if(typeof config.beforeSet === 'function')
                {
                    config.beforeSet(target, name, value, this.path);
                }

                target[name] = GetDeepProxyHelper(value, handler(config), this.path + '.' + name);

                if(typeof config.afterSet === 'function')
                {
                    config.afterSet(target, name, value, this.path);
                }
                return true;
            }
        }
    };

    var GetDeepProxyHelper = function(value, handler, path)
    {
        handler.path = path;
        if(typeof value === 'object')
        {
            for(var prop in value)
            {
                if(typeof value[prop] === 'object')
                {
                    value[prop] = GetDeepProxyHelper(value[prop], handler, path + '.' + prop);
                }
            }
            return new Proxy(value, handler);
        }
        else
        {
            return value;
        }
    }

    return GetDeepProxyHelper(value, handler(config), path);
}

You can then use it like that :

var myConfig = {
    afterSet: function(target, name, value, path)
    {
        console.log('set :', path + '.' + name + ' = ', value);
    },
    beforeSet: function(target, name, value, path)
    {
        //console.log('setting :', path + '.' + name + ' = ', value);
    }
}

var proxy = GetDeepProxy({}, myConfig, 'proxy');

proxy.test = [];
proxy.test.push('hi');
proxy.test[0] = 'abc';
proxy.test.push('def');
proxy.test = proxy.test.slice(1);
proxy.deep = {};
proxy.deep.abc = 'abc';
proxy.deep2 = { abc: { test: {} } };
proxy.deep2.abc.test.value = 'test';

console.log(proxy);
console.log(JSON.stringify(proxy));

Which print the following result

(index):160 set : root.test =  []
(index):160 set : root.test.0 =  hi
(index):160 set : root.test.length =  1
(index):160 set : root.test.0 =  abc
(index):160 set : root.test.1 =  def
(index):160 set : root.test.length =  2
(index):160 set : root.test =  ["def"]
(index):160 set : root.deep =  {}
(index):160 set : root.deep.abc =  abc
(index):160 set : root.deep2 =  {abc: Proxy}
(index):160 set : root.deep2.abc.test.value =  test
(index):180 Proxy {test: Proxy, deep: Proxy, deep2: Proxy}
(index):181 {"test":["def"],"deep":{"abc":"abc"},"deep2":{"abc":{"test":{"value":"test"}}}}

Is there any modifications that I won't be able to observe using this approach and should I structure the code differently?

Missed 2, indirectly blind, know the rules, & chasing the tail.

There are four problems with the functionality. The last one will present you with the biggest challenge, as proxy traps can be CPU costly and adding a check (which involves keeping a reference for each object as you step the path) may well be impractically slow.

The two you missed.

There are two additional method for modifying an object directly

An object can be modified via delete

proxy.test.a = "a";
delete proxy.test.a;  // this modification will be missed

An object can be modified via defineProperty

Object.defineProperty(proxy.test, 'c', {
    writable : false,
    enumerable : false,
    configurable : false,
    value : "definedProp",
});

You will need to use the deleteProperty and defineProperty traps to get them.

The one you don't see.

Also you will not see modification that are indirect.

const testObj = {a : 0, b : 1};
proxy.test[0] = testObj;  // this is trapped
proxy.test[0].c = "c";    // you trap this as well

testObj.c = 10;           // you miss this

The rules.

Though I am not sure if this was the intention but you are also picking up on inherited properties that may have been protected. You need to check that you can write to the property.

Object.defineProperty(Array.prototype, 'foo', {
    writable : false,
    enumerable : false,   // Also not enumerable! should you be looking?
    configurable : false,
    value : "bar",
})
proxy.test = [];
proxy.test.foo = "poo"; // You trap this as if the assignment is ok
// yet
console.log(proxy.test.foo); // outputs "bar"

Chasing the tail.

And the ever present problem that you face when doing deep searches is you have no protection against cyclic references.

It`s a little tricky as i was sure a simple cyclic reference would overflow the call stack, but that did not happen. With a little experimentation I did find a way to make it crash.

const myObj1 = {a : 0};
const myObj2 = {a : myObj1}
proxy.test = [];
proxy.test.push(myObj1)
proxy.test.push(myObj2)
proxy.test[0].a = myObj2;  // all good to this point

// this causes endless iteration and eventually the call stack will throw
// RangeError: Maximum call stack size exceeded
proxy.test[0].b = myObj1;


Loading items for an eCommerce site I am attempting to make an eCommerce site from scratch. Right now I have the following code that gets all the items from a Firebase Firestore database and then adds HTML into an items div element based on the item's info.

var items = [];
var itemsDiv = document.getElementsByClassName("items")[0];

db.collection("items").get().then((querySnapshot) => {
    querySnapshot.forEach((doc) => {
        var docData = doc.data();
        items.push(docData);
    });
    renderItems();
});

function renderItems() {
  console.log(items);
  items.forEach(function(item) {
    itemsDiv.innerHTML += `

      ${item.name}
      ${item.condition}
      ${item.price}

    `
  });
}

What can I do to make my code better?

Here are a few things:

good you're using ` (template literals) to format your HTML inside the JavaScript code
don't use innerHTML += as it forces the browser to re-render the DOM when called. Read more here: Why is Element.innerHTML += bad code?. Instead create an HTML element with .createElement and append it to an existing DOM element .appendChild.

So instead of appending the the HTML string
...
 to the parent div you can:

create a wrapping div element:

const div = document.createElement('div');

append the HTML string to the div wrapper (remember div is an object of type Element but it's not already rendered in the page DOM). You can use all of Element's methods on it, including .innerHTML:

div.innerHTML = `

    ...

`;

then append the wrapper's first child to the parent element:

divParent.appendChild(div);

you can use .querySelectorAll instead of .getELementByClassName. The former allows you to pass in an actual CSS selector instead of a className (the latter method). In your case you only want the first matching element, so use .querySelector instead.
I always use single quotes ' for JavaScript strings and double quotes " when I need quotes inside the strings e.g. with HTML attributes. That's personal preference though.
no need to define the variable docData as doc.data() is already pretty self-explanatory
you can use arrow functions outside also (arg1, arg2, ...) => {} or if you have a single argument arg => {} (no need for the parenthesis).
you could use const for constant variable to make sure they don't get their values changed: for instance itemsDiv (fyi: renamed to itemsParent for clarity) can be defined with const instead of var.

Here's a possible refactoring of your code:

const itemsParent = document.querySelector('.items')[0];
let items = [];

const renderItems = items => {

  items.forEach(item => {

    const div = document.createElement('div');

    div.innerHTML = `

        ${item.name}
        ${item.condition}
        ${item.price}

    `.trim();

    itemsParent.appendChild(div.firstChild);

  });
}

db.collection('items').get().then((querySnapshot) => {

  querySnapshot.forEach(doc => {
    items.push(doc.data());
  });

  renderItems(items);

});

Edit: testing div creation, it appears you need to trim the string first:

const div = document.createElement('div');
div.innerHTML = `

`.trim();

console.log(div.outerHTML)
console.log(div.firstChild.outerHTML)


Calculate the frequency of characters in a string I wrote this program to check the number of times that each letter appears in a string input by the user. It works fine, but is there a more efficient or alternative solution of going about this task than reiterating through a twenty-six-element-long array for every single character?

import java.util.Scanner;
public class Letters {
    public static void main(String[] args) {
        @SuppressWarnings("resource")
        Scanner sc = new Scanner(System.in);
        char[] c = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
        int[] f = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
        System.out.println("Enter a string.");
        String k = sc.nextLine();
        String s = k.toUpperCase();
        s = s.trim();
        int l = s.length();
        System.out.println("Checking string = " + s);
        char ch;
        for (int i = 0; i < l; i++) {
            ch = s.charAt(i);
            for (int j = 0; j < c.length; j++) {
                if (ch == c[j]) {
                    f[j]++;
                }
            }
        }
        System.out.println("Char\tFreq");
        for (int i = 0; i < c.length; i++) {
            if (f[i] != 0) {
               System.out.println(c[i] + "\t" + f[i]);
            }
        }
    }
}

Separate logical elements

It's good to separate different logical parts of a program, for example:

Parse input: a function that takes an InputStream and returns a String
Compute frequencies: a function that takes a String and returns frequencies in some form. In your current program you used an int[], it could have been a Map<Character, Integer>.
Print the frequencies: a function that takes the frequencies in some form, returns nothing, and prints to screen the frequencies nicely formatted.

Computing indexes of letters

If the input string contains only uppercase letters, then you can translate those letters to array indexes in the range of 0 to 25 (inclusive) like this:

int index = ch - 'A';

This eliminates the nested loop you had.
It also eliminates the need for the c array.

Initializing arrays

Instead of this:

int[] f = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

You could write simply int[] f = new int[26];

Instead of this:

char[] c = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};

I would take a lazy approach and write char[] c = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".toCharArray();

Use better variable names

Single-letter variable names should only be used for trivial, highly transient things.
The names f and c in the program are inappropriate, and make the program more difficult to read.


Finding the longest palindromic substring I have the following code that works correctly. I am wondering if there is a simpler way to go about implementing my solution and/or if there is anything that looks non-standard in my code. If anyone wants to offer criticism I'm all ears.

### Question 2 main function and helper functions.
"""Given a string a, find the longest palindromic substring contained in a.
Your function definition should look like question2(a), and return a string."""
# Gives substrings of s in decending order.
def substrings(s):

    # Declare local variable for the length of s.
    l = len(s)

    # Here I chose range over xrange for python version compatibility.
    for end in range(l, 0, -1):
        for i in range(l-end+1):
            yield s[i: i+end]

# Define palindrome.
def palindrome(s):
    return s == s[::-1]

# Main function.
def Question2(a):
    for l in substrings(a):
        if palindrome(l):
            return l

# Simple test case.
print Question2("stresseddesserts")
# stresseddesserts

In terms of efficiency, I like the code.

I'd just fix a few stylistic issues:

Naming: l, a, s don't say anything about what the variable means. Also, you named your function palindrome, but that's returning a boolean, so I'd name it is_palindrome. The i in the substring function is a bit puzzling to me: you have a variable named end, why not call the other one start?
As @kyrill mentioned, you should probably change comments above the functions to docstrings.
The comment Declare local variable for the length of s. is useless.
I'd instead add another comment explaining why it works. If you have a function simply called substrings I expect it to return the substrings. This code only works because it returns the substrings starting from the longest to the shortest. I'd at least change the comment descending order (which may mean for example lexicographical order or alphabetical order) to from longest to shortest. Better be explicit.
I know, it's a bit long, but since you're using a generator, I'd rename the function substrings to get_next_substring.
As @zondo mentioned Question2 is not a PEP8 compliant name, but I'm not sure if the automatic grader will need it to be called like that.


How do I lessen the memory usage of my solution to LeetCode 495: Teemo Attacking How to make changes in my code so that it can take the least memory possible.?
This is a leetcode program , I have tested all the testcases and it has passed, I tried to reduce the memory usage, but by far now, I have not been able to understand.
I want to look into this so that I can understand the usage of memory spaces better. Any input would be helpful. Thank you very much. I will also post the link to the question, but since the logic is already clear to me, The only question is that is this a good solution or memory heavy?
   class Solution {
    public int findPoisonedDuration(int[] timeSeries, int duration) {

        int i =0,old_goalpost=0,new_goalpost=0 ,diff=0;
        int numberOfSeconds = duration;

        if (timeSeries.length >0)
            old_goalpost = timeSeries[0]+duration-1;
        else
            numberOfSeconds=0;

        if (timeSeries.length >0)
        for (i=1;i<timeSeries.length;i++)
        {
            new_goalpost =  timeSeries[i]+duration-1;

            diff = (new_goalpost - old_goalpost)>duration?duration:((new_goalpost - old_goalpost));

            numberOfSeconds = numberOfSeconds +diff;
            old_goalpost = new_goalpost;

        }
        return numberOfSeconds;

    }
}

the question is this

And it has passed all the test cases.

If you want, this is the link to the question
https://leetcode.com/problems/teemo-attacking/
this code is 99.97% faster but memory heavy
class Solution {
    public int findPoisonedDuration(int[] timeSeries, int duration) {

        int i =0,old_goalpost=0,new_goalpost=0 ,diff=0;
        int numberOfSeconds = duration;

        if (timeSeries.length >0)
            old_goalpost = timeSeries[0]+duration-1;
        else
            numberOfSeconds=0;

        if (timeSeries.length >0)
        for (i=1;i<timeSeries.length;i++)
        {
            new_goalpost =  timeSeries[i]+duration-1;
            diff = (new_goalpost - old_goalpost)>duration?duration:((new_goalpost - old_goalpost));
            numberOfSeconds = numberOfSeconds +diff;
            old_goalpost = new_goalpost;

        }
        return numberOfSeconds;

    }
}

Disclaimer: Not a Code Reviewer

Just tested your code, it has a 1ms runtime with 99.97% faster than other solutions.

Don't pay too much attention to these runtime/memory data provided by LeetCode especially when:

it comes to memory; or
test cases are too limited (such as Teemo) because their benchmarkings are not so accurate.

Maybe instead we'd format the code, just a bit making things based on Java's conventions:

public class Solution {
    public static final int findPoisonedDuration(
        final int[] timeSeries,
        final int duration
    ) {

        int i = 0;
        int oldGoalpost = 0;
        int newGoalpost = 0;
        int diff = 0;
        int numberOfSeconds = duration;

        if (timeSeries.length > 0) {
            oldGoalpost = timeSeries[0] + duration - 1;

        } else {
            numberOfSeconds = 0;
        }

        if (timeSeries.length > 0)
            for (i = 1; i < timeSeries.length; i++) {
                newGoalpost =  timeSeries[i] + duration - 1;

                diff = (newGoalpost - oldGoalpost) > duration ? duration : ((newGoalpost - oldGoalpost));

                numberOfSeconds = numberOfSeconds + diff;
                oldGoalpost = newGoalpost;

            }

        return numberOfSeconds;

    }
}

Your solution has an order of N runtime with constant memory O(1).
Algorithmically speaking, which is a key point on solving LeetCode questions, I don't think much can be done to make it any further efficient, I might be wrong though.

public class Solution {
    public static final int findPoisonedDuration(
        final int[] series,
        final int duration
    ) {
        if (series.length == 0 || duration == 0) {
            return 0;
        }

        int totalTime = duration;

        for (int i = 1; i < series.length; i++) {
            totalTime += Math.min(series[i] - series[i - 1], duration);
        }

        return totalTime;
    }
}