Maximum product of 3 integers in an int array using Python - follow up This is a follow up to Maximum product of 3 integers in an int array using Python

Changes to the code include renamed variables for improved readability and I added more test cases.

import unittest

def highest_product(list_of_ints):
    max_seen = [float("-inf"), float("-inf"), float("-inf")]
    min_seen = [float("inf"), float("inf")]

    for x in list_of_ints:
        if x >= max_seen[0]:
            max_seen[0], max_seen[1], max_seen[2] = x, max_seen[0], max_seen[1]
        elif x >= max_seen[1]:
            max_seen[1], max_seen[2] = x, max_seen[1]
        elif x > max_seen[2]:
            max_seen[2] = x
        if x <= min_seen[0]:
            min_seen[0], min_seen[1] = x, min_seen[0]
        elif x < min_seen[1]:
            min_seen[1] = x

    max_product_candidate_one = min_seen[0] * min_seen[1] * max_seen[0]
    max_product_candidate_two = max_seen[0] * max_seen[1] * max_seen[2]

    return max(max_product_candidate_one, max_product_candidate_two)

class TestHighestProduct(unittest.TestCase):
    def test_highest_product(self):
        self.assertEqual(highest_product([6, -1, -1, -2, 0]), 12)
        self.assertEqual(highest_product([-6, -1, -1, -2]), -2)
        self.assertEqual(highest_product([0, 0, 0]), 0)
        self.assertEqual(highest_product([0, 0, -2]), 0)

if __name__ == '__main__':
    unittest.main(verbosity=2)

This version feels much more readable, especialy since you dropped half of your comparisons.

However, I really liked @JoeWallis use of heapq. I would however use heappushpop which will provide all the comparisons you are doing at once.

But you will need to manually extract out the maximum value of max_seen since you have no guarantee anymore on its position.

You can thus write:

from heapq import heappushpop

def highest_product(list_of_ints):
    max_seen = [float("-inf")] * 3
    min_seen = [float("-inf")] * 2

    for x in list_of_ints:
        heappushpop(max_seen, x)
        heappushpop(min_seen, -x)

    # No change of sign since we changed it twice (once for each element)
    max_product_candidate_one = min_seen[0] * min_seen[1] * max(max_seen)
    max_product_candidate_two = max_seen[0] * max_seen[1] * max_seen[2]

    return max(max_product_candidate_one, max_product_candidate_two)


Encrypting messages using the XOR operation recently I've started to learn Clojure. I came up with an easy programming problem, described by this C code:

int main() {
    const int N = 5;
    const int M = 12;

    uint8_t small[N] = { 1, 2, 3, 4, 5 };
    uint8_t big[M];

    memset(big, 0xff, M);

    for(size_t i = 0; i < M; ++i) {
        big[i] ^= small[i % N];
    }

    for(size_t i = 0; i < M; ++i) {
        printf("%02x", big[i]);
        if(i != M - 1)
            printf(" ");
    }

    printf("\n");
    return 0;
}

In other words: I have two arrays. One is a bigger one (my "message" - "big"), and second one is shorter (my "key" - "small"). I would like to encrypt my message with my key, by using the XOR operation. The encryption would perform a bit XOR of every byte from message array with a corresponding byte from the key array. Since the key is shorter than the message, when the key's index will go out of bounds, it should be started from the beginning again.

The message I would like to encrypt is 12 * 255:

255 255 255 255 255 255 255 255 255 255 255 255

And my key is:

1 2 3 4 5

I've came up with this Clojure program:

(defn perform-xor []
  (let [bigmap (repeat 12 0xFF) xorkey [1 2 3 4 5] n (count bigmap)]
    (map (fn [o] (bit-xor (o 0) (o 1)))
         (map vector bigmap (take n (cycle xorkey))))))

(defn get-string-output []
  (interpose
    " "
    (map (fn [b] (format "%02x" b))
         (perform-xor))))

(defn -main [& args]
  (do
    (doseq [strings (get-string-output)]
      (print strings))
    (println)))

I assume it can be written with less lines, and with more Clojure-like approach. Could anyone please tell me what can be done more in the "spirit" of Clojure?

Looks okay, perform-xor can be both simpler and the variables in the
let should be each on a single line.  You don't need the double map,
so just map over bigmap and then you also don't need the take, as
map will only run till one of the arguments runs out, thus:

(defn perform-xor []
  (let [bigmap (repeat 12 0xFF)
        xorkey [1 2 3 4 5]]
    (map (fn [x y] (bit-xor x y))
         bigmap (cycle xorkey))))

The -main function can be simpler, given that you don't (in principle)
care about the garbage from the mapv (mapv because it's eager):

(defn -main [& args]
  (mapv print (get-string-output))
  (newline))

newline just for giggles, or (more seriously) clarifying intent.


A crude clojure progress reporting function The purpose of this function is to report processing progress to the terminal.  It loops through an array of maps that contain two properties: :sequence and :function.  Within each sequence-function-map, it will loop through the sequence, and run the function on each item in the sequence.

This is my first Clojure program that will be used in a production environment.  This particular function does the job, but I suspect that it's not very lispy.

(defn run-with-reporting [sequence-function-maps stop-time]
  (def beginning-time (java.util.Date.))
  (let [seq-counts (map #(count (:sequence %)) sequence-function-maps)]
    (def total-sequence-counts (reduce + seq-counts)))
  (println (str "total counts = " total-sequence-counts))
  (def prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0}))
  (doseq [sequence-function-map sequence-function-maps]
    (let [sequence (:sequence sequence-function-map),
          function (:function sequence-function-map)]
      (loop [loop-seq sequence]

        (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
          (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
          (if loop-seq
            (do
              (def item (first loop-seq))
              (def start-time (java.util.Date.))
              (function item)
              (def end-time (java.util.Date.))
              (def time-delta (- (.getTime end-time) (.getTime start-time)))
              (let [derefd-rpt (deref prog-rpt-ref)]
                (dosync (alter prog-rpt-ref assoc
                               :done (inc (:done derefd-rpt)),
                               :total-time-taken (+ (:total-time-taken derefd-rpt) time-delta))))
              (let [derefd-rpt (deref prog-rpt-ref)]
                (let [average-time (/ (:total-time-taken derefd-rpt) (:done derefd-rpt))]
                  (println "Avg time / each = " (hrs-min-sec average-time)
                           ", Estimated total time left = "
                           (hrs-min-sec (* average-time (- (:todo derefd-rpt) (:done derefd-rpt)))))))
              (recur (next loop-seq))))))))
  (let [derefd-rpt (deref prog-rpt-ref)]
    (println "Total time taken = " (hrs-min-sec (- (.getTime (java.util.Date.)) (.getTime beginning-time))) ", Done = " (:done derefd-rpt) "/" (:todo derefd-rpt))))

Here's a testing function

(defn test-run-w-reporting [stop-clj-time]
  (def testing-func (fn [item] (. java.lang.Thread sleep (rand 1000))))
  (let [sequence-function-maps [{:sequence (range 30), :function testing-func},
                               {:sequence (range 15), :function testing-func}]]
    (run-with-reporting sequence-function-maps stop-clj-time)))

I will point out stuff that I'd do differently, though I am by no means a clojure expert, I do have some experience in it. Numbered items will reappear inside the code as comments.

no DEF inside defn body

def will define the var for your entire namespace as such, you will pollude your namespace with temporaries
use LET instead

no DO necessary inside the body of IF

furthermore, I find it more idiomatic to use WHEN in the case that no else branch exists

using @ instead of DEREF could reduce verbosity
by using UPDATE-IN instead of INC, you can simply pass a transformation function

note the vector notation however: UPDATE-IN takes a vector of keywords

You can pass a partial function, i.e. a function which is
partially applied as the transformation function to update the total-time-taken
No need to nest LETs, you can write all definitions into a single let

Code:

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (java.util.Date.) ;; 1.
        seq-counts (map #(count (:sequence %)) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0})]
    (println (str "total counts = " total-sequence-counts))
    (doseq [sequence-function-map sequence-function-maps]
      (let [sequence (:sequence sequence-function-map),
            function (:function sequence-function-map)]
        (loop [loop-seq sequence]
          (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
            (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
            (when loop-seq ;; 2.
              (let [item (first loop-seq)
                    start-time (java.util.Date.)]
                (function item)
                (let [end-time (java.util.Date.)
                      time-delta (- (.getTime end-time) (.getTime start-time))
                      derefd-rpt @prog-rpt-ref]         ;; 3.
                  (dosync (alter prog-rpt-ref update-in ;; 4.
                                 [:done] inc,
                                 [:total-time-taken] (partial + time-delta))) ;; 5.
                  (let [derefd-rpt @prog-rpt-ref ;; 3. & 6.
                        average-time (/ (:total-time-taken derefd-rpt) (:done derefd-rpt))]
                    (println "Avg time / each = " (hrs-min-sec average-time)
                             ", Estimated total time left = "
                             (hrs-min-sec (* average-time (- (:todo derefd-rpt) (:done derefd-rpt)))))))
                (recur (next loop-seq))))))))
    (let [derefd-rpt (deref prog-rpt-ref)]
      (println "Total time taken = " (hrs-min-sec (- (.getTime (java.util.Date.)) (.getTime beginning-time))) ", Done = " (:done derefd-rpt) "/" (:todo derefd-rpt))))
  )

I would try to reduce verbosity and increase readability (in the
literate sense) by utilising descriptive helper functions -
current-time instead of (.getTime (java.util.Date.))
Personally I like to keep the amount of variables as low as
possible.  Variables are great for producing side-effects,
describing complex results or for when you need a result several
times. In the case of the ref, writing @ instead of using a
temporary seems more natural to me.

As such:

I would try to reduce the amount of side-effects and
use descriptive functions to calculate complex results and place them where they belong
without temporary variables

You can use destructring for maps

(let [{:keys [key1 key2]} mymap] ...) will let-bind key1 and key2 by extracting :key1 & :key2 from mymap

Code:

(defn current-time
  "Return the current time. Same as (.getTime (java.util.Date.))."
  []
  (.getTime (java.util.Date.)))

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (current-time) ;; 1.
        seq-counts (map #(count (:sequence %)) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0})]
    (println (str "total counts = " total-sequence-counts))
    (doseq [sequence-function-map sequence-function-maps]
      (let [{:keys [sequence function]} sequence-function-map] ;; 9.
        (loop [loop-seq sequence]
          (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
            (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
            (when loop-seq ;; 2.
              (let [item (first loop-seq)
                    start-time (current-time)] ;; 7.
                (function item)
                (dosync (alter prog-rpt-ref update-in ;; 4.
                               [:done] inc,
                               [:total-time-taken] (partial + (- (current-time) start-time)))) ;; 5., 7. & 8.
                (let [average-time (/ (:total-time-taken @prog-rpt-ref) (:done @prog-rpt-ref))] ;; 8.
                  (println "Avg time / each = " (hrs-min-sec average-time)
                           ", Estimated total time left = "
                           (hrs-min-sec (* average-time (- (:todo @prog-rpt-ref) (:done @prog-rpt-ref)))))) ;; 8.
                (recur (next loop-seq))))))))
    (println "Total time taken = " (hrs-min-sec (- (current-time) beginning-time))
             ", Done = " (:done @prog-rpt-ref)
             "/" (:todo @prog-rpt-ref))))

Now, lets take a look at the function itself. In general, when
writing in clojure you should try to keep your functions generic
and simple. By keeping them simple and focusing on only a single
task, they are usually easier to compose and reuse.
I would thus recommend splitting up your function.

In your case what you want to do is really:

map a function over a sequence: clojure already provides a function for that, namely MAP.
stop the mapping operation if a specified time has been reached.
print out the average time per call
print out total time taken

The first point is trivial.

For the second point, we can exploit the fact that clojure has lazy
sequences. This allows us to use the built-in TAKE-WHILE function
with a predicate which checks the time and stops when it has passed
your stop-time:

(defn take-until
  "Returns a lazy seq of items from coll util the STOP-TIME has been reached."
  [stop-time coll]
  (take-while
   (fn [item]
     (if (not (> (current-time) stop-time))
       true
       (do (println "stopped at " (current-time) ". Requested stop at " stop-time)
           false)))
   coll))

The third point can be achieved by simply mapping over a collection
and printing out the necessary information. This again is really
just what you did, but refactored into a function that works on any
sequence:

(defn measure-coll-retrieval
  "Returns a lazy seq of items from coll. Will print to stdout the
  average time between element extractions."
  ;; you can use destructuring inside the argument list as well, here
  ;; we're additionaly specifying some defaults for when the caller
  ;; does not provide a value
  [coll & {:keys [start-count total-count] :or {start-count 0 total-count nil}}]
  (let [beginning-time (current-time)]
    (map-indexed
     (fn [index item]
       (let [index (+ start-count index 1)
             average-time (/ (- (current-time) beginning-time) index)]
         (print "Avg time / each = " (hrs-min-sec average-time))
         (if total-count
           (println ", Estimated total time left = "
                    (hrs-min-sec (* average-time (- total-count index))))
           (println))
         item))
     coll)))

Before writing the reporting function, a little helper so we can
get around seq-chunking in clojure:

(defn unchunk
  "takes a chunked sequence and turns it into an unchunked sequence"
  [s]
  (lazy-seq
   (when-let [[x] (seq s)]
     (cons x (unchunk (rest s))))))

Now we can write the actual reporting function:

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (current-time) ;; 1.
        seq-counts (map (comp count :sequence) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        intermed-count (atom 0)]
    (println (str "total counts = " total-sequence-counts))
    ;; note how it is possible to immediately destructure the map
    (doseq [{:keys [sequence function]} sequence-function-maps]
      (swap! intermed-count
             +
             (count
              ;; this is where everything happens: apply function,
              ;; take-until stop-time reached & measure average time
              (measure-coll-retrieval
               (take-until stop-time (map function (unchunk sequence)))
               :start-count @intermed-count
               :total-count total-sequence-counts))))
    (println "Total time taken = " (hrs-min-sec (- (current-time) beginning-time))
             ", Done = " @intermed-count
             "/" total-sequence-counts)))

(defn test-run-w-reporting [stop-clj-time]
  (def testing-func (fn [item] (. java.lang.Thread sleep (rand 1000))))
  (let [sequence-function-maps [{:sequence (range 30), :function testing-func},
                                {:sequence (range 15), :function testing-func}]]
    (run-with-reporting sequence-function-maps stop-clj-time)))

I hope this has helped you some.

Kind regards,
Thomas


c# Converting from one object to another without using foreach I am return an student object from my database and I want to convert it into a StudentRecoredRequest model. The way i have implement is shown below. I was wondering if there was a nicer way to convert from the student object to studentsystemrecordrequest. Thanks
        var result = await _context.Student.ToListAsync();
        var studentInfo = new List<StudentSystemRecordRequest>();
        foreach(var student in result)
        {
            var stud = new StudentSystemRecordRequest
            {
                FirstName = student.FirstName,
                LastName = student.SurName,
                DateOfBirth = student.DateOfBirth,
                Address = student.Address1
            };
            studentInfo.Add(stud);
        }

You can use the LINQ syntax:
var studentInfo =
    (from student in result
     select new StudentSystemRecordRequest
        {
            FirstName = student.FirstName,
            LastName = student.SurName,
            DateOfBirth = student.DateOfBirth,
            Address = student.Address1
        }
    ).ToList();

If you are happy with IEnumerable instead of List, you can omit ToList(), but keep in mind that in that case the resulting enumerable will be lazy, which means that the elements will be created only when the result is enumerated. But since result is probably a list, which is strict, it should not be a problem.


Merge sort implementation: sorting feels clunky I got bored and decided to make a merge sort from scratch.  I like everything, except for my "merging" of the two arrays into a sorted array inside the for loop.  I had several bugs that I had to iron out, which were along the lines of if subArray1 or subArray2 ran out of elements, checking them caused ArrayOutOfBoundsException.  This prompted me to add the two ifs above the sorting to finish the sorting.

Is there a better / less clunky way to "merge" the two arrays together?

import java.util.Arrays;
import java.util.Random;

public class MergeSort {

    public static void main(String[] args) {

        Random random = new Random();
        int max = 10000;
        int arr[] = new int[max];

        for (int i=0; i < max; i++) {
            arr[i] = random.nextInt(2147483647);
        }

        System.out.println("boop!");
        long start = System.currentTimeMillis();
        arr = merge(arr).clone();
        long end = System.currentTimeMillis();

        System.out.println("it took " + (end - start) + " milliseconds");
        System.out.println(Arrays.toString(arr));

    }

    public static int[] merge(int[] array) {

        int maxIndex = array.length;
        int halfIndex = (array.length) / 2;

        // if the array was split into one
        if (array.length <= 1) {
            return array;
        }

        int[] subArray1;
        int[] subArray2;
        int[] sorted;

        subArray1 = Arrays.copyOfRange(array, 0, halfIndex);
        subArray2 = Arrays.copyOfRange(array, halfIndex, maxIndex);

        subArray1 = merge(subArray1);
        subArray2 = merge(subArray2);

        sorted = new int[subArray1.length + subArray2.length];

        int index1 = 0, index2 = 0;
        for (int i = 0; i < sorted.length; i++) {

            // if subArray1 ran out of elements
            if (index1 == subArray1.length) {
                for (; i < sorted.length; i++) {
                    sorted[i] = subArray2[index2++];
                }

                break;
            }

            // if subArray2 ran out of elements
            if (index2 == subArray2.length) {
                for(; i < sorted.length; i++) {
                    sorted[i] = subArray1[index1++];
                }

                break;
            }

            if (subArray1[index1] <= subArray2[index2]) {
                sorted[i] = subArray1[index1++];
                continue;
            }

            if (subArray1[index1] > subArray2[index2]) {
                sorted[i] = subArray2[index2++];
                continue;
            }

        }
        return sorted;

    }

That merge would be clearer as a while (index1 < subArray1.length && index2 < subArray2.length), while bot arrays have elements to be merged in. And you can do 2 System.arraycopys for the remainder afterwards.

while (index1 < subArray1.length && index2 < subArray2.length){
    if (subArray1[index1] <= subArray2[index2]) {
        sorted[i++] = subArray1[index1++];
    else {
        sorted[i++] = subArray2[index2++];
}

System.arraycopy(subArray1, index1, sorted, i, subArray1.length - index1);
i+=subArray1.length - index1;

System.arraycopy(subArray2, index2, sorted, i, subArray2.length - index2);

At the end either subArray1.length - index1 or subArray2.length - index2 would be 0 but then the arraycopy will be a no-op.


JavaScript function to create HTML select option elements I made this function to get rid of writing numerical option values in a HTML-select manually.

From what I have seen so far it works as expected. But hints and suggestions are welcome anyway.

// Trying it out.
var selects = document.querySelectorAll('.drop-down');
selects = Array.prototype.slice.call(selects);

createOptions(selects[0], -9, 9, 8);

for (var i = 1; i < selects.length; i++) {
  createOptions( selects[i],
                 i * (-1 * Math.pow(10, i)),
                 i * Math.pow(10, i));
}

// The actual function ...

// The 1st Parameter is the HTML-element, the
// second is the bottom-limit, the third the
// upper limit.
// The fourth, optional parameter allows to
// define the selected-option.

function createOptions( selectElement,
                        countOptionsStart,
                        countOptionsEnd,
                        preSelected ) {
    var bottom = countOptionsStart + '';
    var top = countOptionsEnd + '';
    var demand; // Count of needed chars.

    preSelected = preSelected || 0;

    var getLeadingZeros = function(demand, value) {
      var chars = value.toString() // Convert to string.
                       .split(''); // Make an array.
      // As long as the count of chars is less then
      // the demanded count iterate ...
      while (chars.length < demand) {
        // If the first element in the array contains
        // an minus ('-') then use splice() for to
        // add a zero at index 1.
        if (chars[0] === '-') {
          chars.splice(1, 0, '0');
        } else {
          // ... otherwise add the zero at the
          // beginning.
          chars.unshift('0');
        }
      }
      // Concat the array-elements to a string and
      // return this string.
      return chars.join('');
    }

    // The longer string is taken.
    demand = bottom.length >= top.length ? bottom.length : top.length;

    // Constructing at adding the option-elements.
   for (var i = countOptionsStart; i <= countOptionsEnd; i++) {
     var option = document.createElement('option');
     var optionInner;
      var innerHTML = i + '';

      if (innerHTML.length < demand)
        innerHTML = getLeadingZeros(demand, innerHTML);

      optionInner = document.createTextNode(innerHTML);
     option.setAttribute('value', i);

     if (i === preSelected) option.setAttribute('selected', 'selected');

     option.appendChild(optionInner);
     selectElement.appendChild(option);
   }
}
<select class="drop-down"></select>
<select class="drop-down"></select>
<select class="drop-down"></select>
<select class="drop-down"></select>

Just a warning: in its current implementation your code works only if countOptionsStart is less than or equal to countOptionsEnd!
So depending on your actual needs you should check this case to exit without generating options, or adapt code to work also in this case.

In the other hand, I suggest you some improvements about process of filling value with zeroes. It dramatically reduces code size, and probably works faster:

// Trying it out.
var selects = document.querySelectorAll('.drop-down');
selects = Array.prototype.slice.call(selects);

createOptions(selects[0], -9, 9, 8);

for (var i = 1; i < selects.length; i++) {
  createOptions( selects[i],
                 i * (-1 * Math.pow(10, i)),
                 i * Math.pow(10, i));
}

function createOptions( selectElement,
                        countOptionsStart,
                        countOptionsEnd,
                        preSelected ) {
  preSelected = preSelected || 0;
  demand = Math.max(
    countOptionsStart.toString().length,
    countOptionsEnd.toString().length);
  for (var i = countOptionsStart; i <= countOptionsEnd; i++) {
    var option = document.createElement('option'),
        strI = Math.abs(i) + '',
        sign = i < 0 ? '-' : '',
        fill = i < 0 ? demand -1 : demand;
    option.setAttribute('value', i);
    if (i === preSelected) option.setAttribute('selected', 'selected');
    option.innerHTML = sign + '0'.repeat(fill - strI.length) + strI;
    selectElement.appendChild(option);
  }
}
<select class="drop-down"></select>
<select class="drop-down"></select>
<select class="drop-down"></select>
<select class="drop-down"></select>


Custom setInterval to run function synchronously This customSetInterval function makes sure that the function inside it is executed synchronously. It also gives that function the ability to determine run or stop the timer. The object returned by customSetInterval has stop function to stop the timer explicitly, later it could have more properties related to the timer status if needed.

What do you think about this function? Are there any possible problems? I am concerned whether it could cause any memory leak issue.

This is my customSetInterval function:

function customSetInterval(func, interval, data, handler = {
  timerId: null,
  stop() {
    return clearTimeout(this.timerId);
  }
}) {
  const isContinue = func(data);

  if (isContinue) {
    handler.timerId = setTimeout(
        customSetInterval,
        interval,
        func,
        interval,
        data,
        handler
    );
  }

  return handler;
}

Not much to review but
A few points
Too many arguments. You have a lot of arguments that could actually be passed as  just the handler, and if you make the timeout callback one declared in the customSetInterval (rather than call customSetInterval) you can close over some of the variables and thus would not have to pass them (and keep them protected).
The variable isContinue could be removed as it is only used once and may be better as
if (func(data)) {
    handler.timerId = setTimeout(...

The first call to customSetInterval executes the function immediately, this is not what setTimeout does. On the first pass should it not set a timeout rather than execute?
Memory leaks?
That would be up to the functions being called. Your timeout will eventually release any references that are held pending the timeout so you are not causing a leak. There could be memory problems (not a leak just running low) if there are a lot of timeouts and the delay between them is long, but not a real concern.
Timing issues.
You set the timeout after you call the function and do not account for the time spent running the function. The timeouts will always be late and any error in time will accumulate with each call by the amount of time the func takes to execute (and any other page blocking events).
The way to deal with this is to record the start time and adjust the timeout to be at the correct interval.
That then introduces the issue of catching up to late calls. There are any number of ways the page can blocked making your calls is late. If late do you still make the call or skip it. If you make the call and the function you call is taking longer than the interval do you create a stack of pending calls?
A quick rewrite
As a suggestion only to demonstrate the use of closure to keep stats on the timeout and keeping track of the next scheduled time out, adjusting the time to keep things on time if possible (if more than half a interval behind the timeout is skipped) (though I did not test that)

function customSetInterval(handler) {
  var id, interval, count;
  const startTime = performance.now();

  // this returns the time till next interval. If late will drop calls.
  function getNextCallTime() {
    var nextCallIn = (startTime + interval * (count + 1)) - performance.now();
    if (nextCallIn < -interval / 2) { // to late drop the call
      count = Math.floor((performance.now() - startTime) / interval) + 1;
      nextCallIn = (startTime + interval * count) - performance.now();
    }
    return nextCallIn;
  }

  // This handles the callback
  function execute(handler) {
    if (handler.func(handler)) {
      count += 1;
      id = setTimeout(execute, getNextCallTime(), handler);
    }
  }
  handler = Object.assign({
    interval: 1000,
    stop() {
      return clearTimeout(id)
    },
  }, handler);
  count = 0;
  interval = handler.interval;
  id = setTimeout(execute, getNextCallTime(), handler);
  return handler;
}

// Tests as examples how to.

function run(handler) {
  log("Tick for " + handler.data.name);
  handler.data.counter -= 1;
  if (handler.data.counter === 0) {
    log("Run complete for " + handler.data.name, "red");
    return false;
  }
  return true;
}

function killNumThree(handler) {
  log("Assasin Killing number 3");
  handler.data.handleFor3.stop();
  log("Run complete for " + handler.data.name, "red");
  log("No more intervals expected!", "red");
}

// every second ten times
customSetInterval({
  func: run,
  interval: 1000,
  data: {
    counter: 10,
    name: "Number ONE"
  }
});

// every half second 10 times
customSetInterval({
  func: run,
  interval: 500,
  data: {
    counter: 10,
    name: "Number Two"
  }
});

// every 10th second 20 times
customSetInterval({
  func: run,
  interval: 100,
  data: {
    counter: 20,
    name: "Number Two and a half"
  }
});

// every 2.5 seconds 1000 times
const num3Handle = customSetInterval({
  func: run,
  interval: 2500,
  data: {
    counter: 1000,
    name: "Number Three"
  }
});

// once at 11 seconds and stops number 3
customSetInterval({
  func: killNumThree,
  interval: 11000,
  data: {
    name: "Assasin",
    handleFor3: num3Handle
  }
});

var last;

function log(data, col = "black") {
  var div = document.createElement("div");
  div.textContent = data;
  div.style.color = col;
  if (!last) {
    document.body.appendChild(div);
  } else {
    document.body.insertBefore(div, last);
  }
  last = div;
}


Counting duration values I have a durations as such:

[{'duration': 3600}, {'duration': 3600}]

And I want the output to be {3600: 2}, where 2 is the number of occurrences of 3600.

My first attempt is use a loop, as such:

    var count = {};
    for (var d of durations) {
        var length = d.duration;
        if (length in count) {
            count[length] += 1;
        }
        else {
            count[length] = 1;
        }
    }
    console.log(count);

The second attempt uses reduce() in lodash:

    function foo(result, value) {
        var length = value.duration;

        if (length in result) {
            result[length] += 1;
        }
        else {
            result[length] = 1;
        }

        return result;
    }
    var reduced = _.reduce(durations, foo, {});
    console.log(reduced);

As can be seen, this second attempt is still as verbose as before.

Is there a way to write the iteratee function foo more conform to functional programming?

It's not really clear what you mean by "functional".

The use of reduce is nifty, and correctly done, but it seem like cargo cult programming. The realization that a lot of loop-like actions can be represented in terms of reduce (fold, aggregate, etc) is important to functional programing, but it should no more be your first choice than a while-loop.

The fact that it's verbose isn't necessarily a problem. It suggests that you might be failing at a general goal:

  Write what you mean.

One of the things that makes functional programming good is that it helps us do that.
Javascript has limited tools for directly declaring lists. What you mean to do is declare a Dictionary from the Set of duration values to the Count of the source array Filtered by the respective duration. Or even better you could use a grouping function.

You could bring in a library for the task, which may be a good idea, or you could compromise a little.

function getCounts(items, key){
  let c = {};
  for(let item of items){
    c[key(item)] = items.filter(function(i){
        return key(i) === key(item);
      }).length;
  }
  return c;
};
counts = getCounts(durations, function(i){ return i['duration']; })'

You'll notice that that's quite inefficient. We could make it less wasteful, but it'd be less terse.


using method missing to reduce the code in ruby I have a set of Class methods which is :

class << self
def increment_value
  self.first.increment_value
end

def max_work_hours_per_day
  self.first.max_work_hours_per_day
end

def fast_completion_day
  self.first.fast_completion_day
end

def super_fast_completion_day
  self.first.super_fast_completion_day
end

def ludicrous_completion_day
  self.first.ludicrous_completion_day
end

def budget_completion_day
  self.first.budget_completion_day
end
end

Since all the methods call self.first.atttibute where attribute is the same name as function name. I think we can reduce this to a single method. Something like method_missing is there in Ruby , but since I am not very good at meta programming , I am seeking advise here.

Ruby already has a module which allows you to forward specific methods to a given object in its standard library: the Forwardable module.

Using it you can write code like this:

class << self
  extend Forwardable

  def_delegators :first, :increment_value, :max_work_hours_per_day #...
end

I think spelling out the methods you want to forward like this is preferable to simply forwarding everything as it won't accidentally forward something you don't want to forward. It also does not affect the error message when calling a method that does not exist.


Is this a valid loop? I have the following code:

private ScatterViewItem FindScatterViewOfSourceFile(SourceFile find)
{
   foreach (ScatterViewItem svi in classScatterViews)
   {
      if ((svi.Tag as SourceFile).Equals(find))
      {
         return svi;
      }
   }
   return null;
}

Now I'm asking myself if this is valid or if I should better use:

private ScatterViewItem FindScatterViewOfSourceFile(SourceFile find)
{
   ScatterViewItem result = null;
   foreach (ScatterViewItem svi in classScatterViews)
   {
      if ((svi.Tag as SourceFile).Equals(find))
      {
         result = svi;
         break;
      }
   }
   return result;
}

Is there any common practive which one to use? And are both loops doing the same?

First one is hundred times better then the second one. I would avoid defining a variable if you don't really need to. Also I don't believe in one return methods. Especially I don't believe that they improve readability.

LINQ would definitely improve it:

return classScatterViews.FirstOrDefault(v => v.Tag.Equals(find));

Also I would not use as operator if I do not check result for null.


Little command line video grabber I made a little Python3 script to grab video clips from a website for viewing off-line later and would like your critique, advice, tips & tricks and/or learning resources to create better code. (Project on GitHub)

#!/usr/bin/env python3
''' phgrab.py: clip downloader. '''

import os
import sys
import argparse
import urllib.request
import bs4

def reporthook(blocknum, blocksize, totalsize):
    ''' Define the reporthook for url-retrieval
    (displays info while grabbing). '''
    readsofar = blocknum * blocksize
    if totalsize > 0:
        percent = readsofar * 1e2 / totalsize
        display = "\r%*d / %d %5.0f%%" % (
            len(str(totalsize)), readsofar, totalsize, percent)
        sys.stdout.write(display)
        if readsofar >= totalsize:
            sys.stdout.write("\n")
    else:   # total size unknown
        sys.stderr.write("read %d\n" % (readsofar,))

PARSER = argparse.ArgumentParser(
    description="a commandline clip downloader.")
PARSER.add_argument('clipcode', nargs='+',
                    help="a ten-digit clipcode.")
PARSER.add_argument('-p', '--proxy',
                    help="A http proxy to use; e.g. http://localhost:8000")

PROXY = PARSER.parse_args().proxy
CLIPCODES = PARSER.parse_args().clipcode

if PROXY:
    print("Grabbing through http proxy: %s\n" % PROXY)
    os.environ['http_proxy'] = PROXY

for i in CLIPCODES:
    if not i.isdigit() or not len(i) == 10:
        sys.exit(
            "'\033[1m%s\033[0m' is not a valid ten-digit clipcode," % i,
            "please check your input and try again.")

for i in CLIPCODES:
    soup = bs4.BeautifulSoup(
        urllib.request.urlopen("http://somesite.com/%s" % i))
    title = soup.find("h1", class_="video-title").string
    href = soup.find("a", class_="download button")['href']
    print("Grabbing \033[1m%s\033[0m:" % i)
    local_filename, headers = urllib.request.urlretrieve(
        href, '%s - %s.mp4' % (i, title), reporthook)
    print("\033[1m", local_filename, "Done\033[0m")

A bug

This just won't work:

sys.exit(
    "'\033[1m%s\033[0m' is not a valid ten-digit clipcode," % i,
    "please check your input and try again.")

sys.exit takes a single integer parameter to use as the status code of the program, so that callers can check if the program terminated normally or failed.
I suppose you wanted to print instead and then sys.exit(1).

Move all code to methods

Don't keep code in the global namespace.
A good practice is to move everything to methods,
and call them from inside a if __name__ == "__main__": guard, like this:

def parse_args():
    # arg parser code

def main():
    args = parse_args()
    proxy = args.proxy
    clipcodes = args.clipcode

if __name__ == "__main__":
    main()

This way it will be possible to import the module into a larger app later.
Another advantage is that it doesn't pollute the global namespace,
which could lead to obscure bugs.

Use print() if you want to print

Printing with sys.stdout.write is very unusual and tedious.
Using print() is more common and simple.
If you don't want to print a newline, use the end='' flag, for example:

print(message, end='')

Naming

i is really a very poor name in this loop:

for i in CLIPCODES:
    if not i.isdigit() or not len(i) == 10:
        # ...

It would be better to use something more descriptive, for example clipcode.

Another thing here, not len(i) == 10 is a very odd way of writing len(i) != 10.

It's very unusual to see ALL_CAPS for variable names that are not global constants.
For example DEFAULT_PROXY_PORT = 8080 would make a fine global constant,
but these look really odd:

PARSER = argparse.ArgumentParser(...)
CLIPCODES = PARSER.parse_args().clipcode


Generating classes from enum I have a system that receives messages. Each message has a type defined. These types are declared in an enum:

enum MessageType {
  TYPE1, TYPE2, ... , TYPE999
}

People can extend my system by implementing message processors. Each message type can only have one processor and most will have none. My current design has users writing their processors and I have one factory that they have to change:

public static Map<MessageType, MessageProcessor> getProcessors() {
  Map<MessageType, MessageProcessor> processors = Maps.newHashMap();
  processors.put(TYPE33,  new Type33Processor());
  processors.put(TYPE111, new Type111Processor());
  processors.put(TYPE222, new Type222Processor());
  processors.put(TYPE444, new Type444Processor());
  return processors;
}

My problem with this is twofold:

Each new processor has to remember to add this boilerplate.
This factory has to know all processor types.

I thought of maybe solving this using reflection:

public static Map<MessageType, MessageProcessor> getProcessors() {
  Map<MessageType, MessageProcessor> processors = Maps.newHashMap();
  Reflections reflections = new Reflections(getClass().getPackage().getName());
  Set<Class<? extends MessageProcessor>> allProcessors =
      reflections.getSubTypesOf(MessageProcessor.class);
  for (Class<? extends MessageProcessor> processorClass : allProcessors) {
    MessageProcessor processor = processorClass.getConstructor().newInstance();
    processors.put(processor.getMessageType(), processor);
  }
  return processors;
}

This solution solves the two issues I mentioned above.
My questions are:

I understand that reflection has dangers and pitfalls. Is this a valid scenario to use it?
Is there a better way to improve my first solution without having to resort to using reflection?

I understand your concerns here. This is a messy situation, and Java (like most languages, I believe), makes it relatively hard to build up interdependencies during the bootstrapping of the classes, classloaders, etc.

What would be ideal is if you could have an Enum that allowed the following:

MessageProcessor processor = message.getMessageType().getProcessor();

but, that would require the MessageType to be constructed knowing what it's processor is.... but you need to map that on a case-by-case basis, based on the factory that supplies them.

What is certainly not ideal, is having public static methods that create new Maps each time they are called....

public static Map<MessageType, MessageProcessor> getProcessors() { ...

Why does it need to be public... what happens if it is called multiple times?

That's an ugly solution.

The ideal solution would be to construct the MessageProcessor at the same time as the MessageType, but, since that can't be done (as the MessageType is a parameter to the factory), I would instead recommend that you create a synchronized, or locked, or atomic method that is part of the Enum, and handles the situation for you. If your factory has the method:

public MessageProcessor getMessageProcessorFor(MessageType mType) {
    .....
}

then your enum could have a private setup like;

enum MessageType {
    TYPE1, TYPE2, ... , TYPE999;

    private final AtomicReference<MessageProcessor> processor = new AtomicReference<>();

    public MessageProcessor getProcessor() {
        MessageProcessor proc = processor.get();
        if (proc != null) {
            return proc;
        }
        proc = FactoryClass.getmessageProcessorFor(this);
        if (processor.compareAndSet(null, proc)) {
            return proc;
        }
        // there was a race condition, we lost, so use the winner
        return processor.get();
    }
}

This is a compromize I use quite often. It gives you an abstraction between the class-load initialization, and the runtime usage of the enum.

It has the following benefits:

you have a simpler run-time execution model.
your exceptions, if any, related to creating the MessageProcessor have better traces.
you do not have complicated circular dependencies at construct time.
you only create instances for types you use.

on the down-side, you have the following:

slight performance hit each time you call the method as it checks its state.
well, no 2, really.


Better way to get the XML attribute value? I have an xml file from which I want to extract the value of attribute custName from the very first child. The code below works where I am using the dom parser. Is there a third-party library with which I can do the same with less code and more neatly?

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import org.w3c.dom.Document;
import org.w3c.dom.Node;

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
    DocumentBuilder db = dbf.newDocumentBuilder();
    Document document = (Document) db.parse(new File(
        tempDir));
    Node node = ((Document) document).getFirstChild();
    String custName= node.getAttributes()
        .getNamedItem("custName")
        .getNodeValue();
    assertEquals( "scott",custName);

Update:- Did the above approach use DOM4J (sorry but I am not sure)?

You could use the Java XPath API:

import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathFactory;
import org.xml.sax.InputSource;

XPath xpath = XPathFactory.newInstance().newXPath();
InputSource inputSource = new InputSource(???); // ??? = InputStream or Reader
String custName = xpath.evaluate("//*[1]/@custName", inputSource);

There is a lot more information in this article: http://www.ibm.com/developerworks/library/x-javaxpathapi/index.html


LINQ filtering query I often find myself writing LINQ queries like this.  I end up testing for Null and zero-counts pretty often, and the code sprawls out to be something like the following:

// GET: api/CustomBehaviours
public IEnumerable<CustomBehaviour> Get(int? orgID = null, long? userID = null)
{
    using (PAMDatabase db = new PAMDatabase(WebApiConfig.PAMConnectionString))
    {
        List<CustomBehaviour> output = new List<CustomBehaviour>();

        // Add all custom behaviours for the specified user
        if (userID != null) {
            var usr = db.context.users.AsNoTracking().Where(u => u.userID == userID).FirstOrDefault();
            if (usr != null)
            {
                var cbs = usr.customBehaviours.ToList();
                if ((cbs != null) && (cbs.Count > 0))
                    output.AddRange(Mapper.Map<List<CustomBehaviour>>(cbs));
            }
        }

        // Add all custom behaviours for the specified organisation
        if (orgID != null)
        {
            var org = db.context.orgs.AsNoTracking().Where(o => o.orgID == orgID).FirstOrDefault();
            if (org != null)
            {
                var cbs = org.customBehaviours.ToList();
                if ((cbs != null) && (cbs.Count > 0))
                    output.AddRange(Mapper.Map<List<CustomBehaviour>>(cbs));
            }
        }

        return output;
    }
}

Can this be made more concise?  I'm tempted to remove all the Null checks and wrap the whole thing in a big try { } catch {} but I believe that goes against the conventional use of exceptions.

FirstOfDefault can also take a filtering query, just so you know.

Which means :

db.context.users.AsNoTracking().Where(u => u.userID == userID).FirstOrDefault();

can be :

db.context.users.AsNoTracking().FirstOrDefault(u => u.userID == userID);

If you use C#6, you can use the null propagation operator ?. instead of multiple null checks :

if (usr != null)
{
    var cbs = usr.customBehaviours.ToList();
    if ((cbs != null) && (cbs.Count > 0))
        //...

is now :

if (usr?.customBehaviours != null && cbs.Count > 0)

As pointed @ANeves in the comments, ToList never returns null. It'll return an empty collection or will throw an ArgumentNullException if the enumerable is null.

So, doing .ToList() before checking if usr.customBehavious == null won't save you.

I feel like you should split both your if in different methods, maybe GetUserCustomBehavior and GetOrganizationCustomBehavior. Then, your code woudl be cleaner.

// GET: api/CustomBehaviours
public IEnumerable<CustomBehaviour> Get(int? orgID = null, long? userID = null)
{
    using (PAMDatabase db = new PAMDatabase(WebApiConfig.PAMConnectionString))
    {
        List<CustomBehaviour> output = new List<CustomBehaviour>();

        // Add all custom behaviours for the specified user
        if (userID != null) {
            var userBehaviors = GetUserCustomBehaviors(userID.Value, db);
            output.AddRange(Mapper.Map<List<CustomBehaviour>>(userBehaviors));
        }

        // Add all custom behaviours for the specified organisation
        if (orgID != null)
        {
            var orgBehaviors = GetOrganizationCustomBehavior(orgID.Value, db);
            output.AddRange(Mapper.Map<List<CustomBehaviour>>(orgBehaviors));
        }

        return output;
    }
}

private IEnumerable<???> GetUserCustomBehavior(int? userId, PAMDatabase db)
{
    var usr = db.context.users.AsNoTracking().FirstOrDefault(u => u.userID == userID);

    return usr?.customBehaviours ?? new List<???>();
}

private IEnumerable<???> getOrganizationCustomBehavior(int orgId, PAMDatabase db)
{
    var org = db.context.orgs.AsNoTracking().FirstOrDefault(o => o.orgID == orgID);

    return org?.customBehavior ?? new List<???>();
}

Now, notice something, I wasn't able to see which type was used in your code because of the var keyword. var is cool, but tools like Resharper indicate they should be used only when you can see which type is used ex :

//good
var list = new List<int>();
//not good
var user = FooTheBar();

Also, you don't really need to check for Count > 0, what's the worst that can happen, you'll add an empty range to your list? That's usually not important, and your code has less if, which is good.

I'm no pro with Automapper, but I don't think it's a good practice to map lists, maybe you should define your map per object (Mapper.CreateMap<???,CustomBehaviour>()) but maybe there's a performance gain I'm not aware of.


Raising an error for slug unique in django I have the following model in models.py:

class Question(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField(max_length=100, db_index=True, unique=True)

for which i use a generic view:

class QuestionCreate(generic.CreateView):
    model = Question
    fields = ['title', 'text']
    def form_valid(self, form):
        instance = form.save(commit=False)
        slug = slugify(form.cleaned_data['title'])
        if Question.objects.filter(slug=slug).exists():
             form.add_error("title","This Question already exist")
             return self.form_invalid(form)

The idea is to automatically create a slug from the users title to use for url, is this approach okay if i want to do this? Is the way i added the error message correct?

Edit: I didn't realize that you did not want to save the form to the database. It's obvious now, sorry. So this answer no longer applies. But why don't you try to save the actual Question object instead?

This should work, and there's no reason to change if you validated that it does work with tests.

However it would be more Pythonic to actually try saving (without commit=False), then catching the resulting django.db.IntegrityError exception. See EAFP to understand why this is more Pythonic. The happy path will be faster too since you only need to call the database once.


Express error if agency not found middleware In an api I'm building, I have multiple routes that require an agency to exist before any action is performed on the requested agency.

To avoid repetition I've written some middleware to check that the agency does indeed exist before continuing (and throws if it does not).

The code I have works but I have to make two requests to the db, I'm wondering if I can accomplish the same goal with only a single query.

Here is my code:

// routes/v1/index.js

const express = require("express")
const router = require("express-promise-router")()

router.use(["/agencies/:id", "/agencies/:id/*"], agencies.checkExistsByReqId)

router
  .route("/agencies/:id")
  .get(agencies.getById)
  .patch(agencies.patch)
  .delete(agencies.delete)

export default router

// controllers/v1/agencies/checkExistsByReqId.js

import db from "../../../db"
import { AgencyDoesNotExistError } from "../../../errors"

export default async (req, res, next) => {
  // note: To make the query as small as possible this just returns an id if exists
  const exists = await db.agency.checkExistsById(req.params.id)

  if (!exists) throw new AgencyDoesNotExistError()

  next()
}

// /controllers/v1/agencies/getById.js

import db from "../../../db"
import { AgencyDoesNotExistError } from "../../../errors"

export default async (req, res) => {
  const agency = await db.agency.getById(req.params.id)

  const isMember = agency.members.find(member => member.authId === req.user.sub)
  if (!isMember) throw new AgencyDoesNotExistError()

  res.send(agency)
}

// db/index.js

export const agency = {
  create: async data => new Agency(data).save(),
  findOne: filter => Agency.findOne(filter),
  findOneAndUpdate: (filter, update, options) =>
    Agency.findOneAndUpdate(filter, update, options),
  getById: async agencyId => Agency.findById(agencyId).populate("members"),
  checkExistsById: agencyId => Agency.findById(agencyId, "_id")
}

I managed to accomplish this by adding the agency to res.locals. Here's the amended code:

// /routes/v1/index.js

const router = require("express-promise-router")()

router.use(["/agencies/:id", "/agencies/:id/*"], agencies.addAgencyToResLocals, agencies.ensureUserIsMember)

router
  .route("/agencies/:id")
  .get(agencies.getById)
  .patch(agencies.ensureUserIsAdmin, agencies.patch)
  .delete(agencies.ensureUserIsAdmin, agencies.delete)

// /controllers/v1/agencies/addAgencyToResLocals.js

import db from "../../../db"
import { AgencyDoesNotExistError } from "../../../errors"

export default async (req, res, next) => {
  const agency = await db.agency.getById(req.params.id)

  if (!agency) throw new AgencyDoesNotExistError()

  res.locals.agency = agency

  next()
}

// /controllers/v1/agencies/ensureUserIsMember.js

import { UnauthorizedError } from "../../../errors"

export default (req, res, next) => {
  const { agency } = res.locals

  const isMember = agency.members.find(member => member.authId === req.user.sub)

  if (!isMember) throw new UnauthorizedError()

  next()
}

// /controllers/v1/agencies/getById.js

export default async (req, res) => res.send(res.locals.agency)

I'm not sure if this is the best solution but it simplifies my code a lot. The one issue I see is that another person working with the code base for the first time would have to have knowledge about the middleware being applied. That being said I feel it's acceptable to say that someone working with express should have knowledge of middleware and how it works IMO.


Loading template file and replacing placeholders I have an HTLP template file called "template.tmpl". In the template I have a few placeholders like {{NAME}} and {{USERNAME}}. I am then loading the template file into a variable called $template using file_get_contents. I then put the $template variable through a function which deals with replacing a series of placeholders.

My code is below and seems to be working fine, but wanted some input if there was any way to improve or make it more efficient.

function replace_tags($string){
    $vars = array('{{NAME}}'=>'Bob', '{{USERNAME}}'=>'user_name', '<?'=>'', '?>'=>'');
    return str_replace(array_keys($vars), $vars, $string);
}

$template = file_get_contents('template.tmpl', true);

echo replace_tags($template);

I would say suggest writing your input like so:

function replace_tags($template, $placeholders){
    $placeholders = array_merge($placeholders, array('<?'=>'', '?>'=>''));
    return str_replace(array_keys($placeholders), $placeholders, $template);
}

$vars = array('{{NAME}}'=>'Bob', '{{USERNAME}}'=>'user_name');
$template = file_get_contents('template.tmpl', true);

echo replace_tags($template, $vars);

So that you can define the constant changes, and the changes you want to build depending on user, etc.


Function to find text between two tags I've just finished this function and wanted to know if anyone know another way to do the same:

function findtext(archivo,Delimit1, Delimit2 :String) :String;
var
  Buffer      :AnsiString;
  ResLength   :Integer;
  i           :Integer;
  PosDelimit  :Integer;
begin
  Buffer := read_file_z(archivo);
  if Pos(Delimit1, Buffer) > Pos(Delimit2, Buffer) then
    PosDelimit := Length(Buffer)-(Pos(Delimit1, Buffer)+Length(Delimit1))
  else PosDelimit := Length(Buffer)-(Pos(Delimit2, Buffer)+Length(Delimit2));
  Buffer := Copy(Buffer, (Length(Buffer)-PosDelimit), Length(Buffer));
  ResLength := Pos(Delimit2, Buffer)-(Pos(Delimit1, Buffer)+Length(Delimit1));
  for i := 0 to (Reslength-1) do
    Result := Result+Buffer[Pos(Delimit1, Buffer)+(Length(Delimit1)+i)];
end;

The auxiliary function read_file_z is this:

function read_file_z(const FileName: String): AnsiString;
var
  F: File;
  DefaultFileMode: Byte;
begin
  DefaultFileMode := FileMode;
  try
    FileMode := 0;
    AssignFile(F, FileName);
    {$I-}
    Reset(F, 1);
    {$I+}
    if IoResult=0 then
      try
        SetLength(Result,FileSize(F));
        if Length(Result)>0 then begin
          {$I-}
          BlockRead(F,Result[1],LENGTH(Result));
          {$I+}
          if IoResult<>0 then Result:='';
        end;
      finally
        CloseFile(F);
      end;
  finally
    FileMode := DefaultFileMode;
  end;
end;

The function finds text between two tags. An example would [hi]hi world[hi] and the function will respond "hi world".

Example:

findtext('test.exe','[hi]','[hi]');

The way seek to do the function is the "uses" default.

What alternatives do I have to make the function "findtext"?

You code is extremly hard to read. But I belive this is what you are looking for:

function FindText(FileName: TFilename; const Delimit1, Delimit2: string): string;
var
  Buffer: TStringList;
  PD1, PD2: Integer;
begin
  Buffer := TStringList.Create;
  try
    Buffer.LoadFromFile(FileName);

    PD1 := Pos(Delimit1, Buffer.Text) + Length(Delimit1);
    PD2 := Pos(Delimit2, Buffer.Text);

    Result := Copy(Buffer.Text, PD1, PD2 - PD1);
  finally
    FreeAndNil(Buffer);
  end;
end;


Checking if the whole number is prime, and if all its digits are too That was an easy one to solve but the problem lies in the efficiency since it checks every single digit of the number while converting to integers. I'm receiving TLE (i.e. Time Limit Exceeded) on the site so I have to increase its performance.

So its hard to explain but the exercise asks for the return value to be "Super" when the number is prime and all of its digits are too, return "Primo" when the number is prime but its digits are not, return "Nada" When the whole number is not prime. The test cases end with EOF.

Its worth mentioning that only built-in modules are accepted by the site. I tried using the well known (and much faster) sympy.isprime method, but it's from a 3rd party module so it wasn't accepted, sadly.

Again, I solved it, but I need a bump in the efficiency since there are 1018 numbers in the test cases and more than 200 test cases and I need to solve it in 2 seconds:

def is_prime(n):
    if n in range(0, 2):
        return False
    for i in range(2, n):
        if n % i == 0:
            return False
    return True

while True:
    try:
        data = input()
    except EOFError:
        break
    if is_prime(int(data)):
        for j in data:
            if not is_prime(int(j)):
                print("Primo")
                break
        else:
            print("Super")
    else:
        print("Nada")

The easiest improvement to make is to only go up to n**.5 inisprime This yields a quadratic speedup. The next obvious improvement is to not convert each digit to an int, but rather just use if j not in "2357":. This saves time in two ways: First, it skips an int conversion, and secondly, it uses one check instead of a for loop.


Insertion Sorting in C++ From Wikipedia:

  Insertion sort iterates, consuming one input element each repetition,
  and growing a sorted output list. Each iteration, insertion sort
  removes one element from the input data, finds the location it belongs
  within the sorted list, and inserts it there. It repeats until no
  input elements remain.

  Sorting is typically done in-place, by iterating up the array, growing
  the sorted list behind it. At each array-position, it checks the value
  there against the largest value in the sorted list (which happens to
  be next to it, in the previous array-position checked). If larger, it
  leaves the element in place and moves to the next. If smaller, it
  finds the correct position within the sorted list, shifts all the
  larger values up to make a space, and inserts into that correct
  position.

Just tried out my solution in C++:

#include <vector>

std::vector<int> ins_sort(std::vector<int> series)
{
    for(int i=1; i<series.size(); i++)
    {
        int key = series[i];
        for(int j=i-1; j>-1; j--)
        {
            if (key < series[j])
            {
                series[j+1] = series[j];
                series[j] = key;
            }
            else break;
        }
    }
    return series;
}

I wouldn't recommend passing and returning the vector by value. Pass by reference, and do not return at all - typically sorting is done in place anyway.

Another generic advice: no raw loops. The internal loop does some important job, which has to be realized and given a proper name, insert perhaps.

typedef std::vector<int>::size vsize_t;
void ins_sort(std::vector<int> & series)
{
    for(vsize_t i=1; i < series.size(); i++)
    {
        insert(series, i, series[i]);
    }
}

void insert(std::vector<int> & series, vsize_t last, int key)
{
    for (vsize_t j = last - 1; j > -1; --j)
    {
        if (key < series[j]) {
            series[j + 1] = series[j];
            series[j] = key;
        }
        else
            break;
    }
}

Now it is possible to simplify the logic by eliminating j, and combining the two condition checks together:

void insert(std::vector<int> & series, vsize_t last, int key)
{
    while ((--last > -1) && (key < series[last]))
    {
        series[last + 1] = series[last];
        series[last] = key;
    }
}

You can see how the series[last] = key; assignment is rather redundant - it will necessarily be overwritten at the next iteration, so it only makes sense only at the last one:

void insert(std::vector<int> & series, vsize_t last, int key)
{
    while ((--last > -1) && (key < series[last]))
    {
        series[last + 1] = series[last];
    }
    series[last] = key;
}

Testing two conditions at every iteration is expensive. You may notice that the first condition only strikes if key is less than all the elements, in other words, it is less than series[0] (it is sorted up to the point already):

void insert(std::vector<int> & series, vsize_t last, int key)
{
    if (key < series[0]) {
        while (last-- > 0) {
            series[last+1] = series[last];
        }
    } else {
        while (key < series[last])
        {
            series[last+1] = series[last];
        }
    }
    series[last] = key;
}

and applying the rule one more time, we see that the first loop does shift right by 1, and the second does unguarded shift right by one.

Combining everything, we are getting:

typedef std::vector<int>::size vsize_t;
vsize_t unguarded_shift(std::vector<int> & series, vsize_t last, int key)
{
    while (key < series[last--])
    {
        series[last + 1] = series[last];
    }
    return last;
}

vsize_t shift(std::vector<int> & series, vsize_t last)
{
    while (last-- > 0) {
        series[last+1] = series[last];
    }
    return last;
}

void insert(std::vector<int> & series, vsize_t last, int key)
{
    if (key < series[0]) {
        last = shift(series, last);
    } else {
        last = unguarded_shift(series, last, key);
    }
    series[last] = key;
}

void ins_sort(std::vector<int> & series)
{
    for(vsize_t i=1; i < series.size(); i++)
    {
        insert(series, i, series[i])
    }
}


Is this Android click handler using threads correctly? I am implementing a thread like the following.  Is my implementation correct?

It works well, but I need a check.

myList.setOnItemClickListener(new OnItemClickListener() {

    @Override
    public void onItemClick(AdapterView<?> parent, final View view,
            int position, long id) {
        // TODO Auto-generated method stub
        Runnable runnable = new Runnable() {

            @Override
            public void run() {
                // TODO Auto-generated method stub
                Looper.prepare();
                Context context = view.getContext();

                final String      titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
                final String        descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
                final Bitmap        bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();

                Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();
                myHandler.post(new Runnable() {

                    @Override
                    public void run() {
                        // TODO Auto-generated method stub

                        Intent intent = new Intent(getApplicationContext(), ItemDetails.class);

                        ByteArrayOutputStream stream = new ByteArrayOutputStream();
                        bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
                        byte[] bytes = stream.toByteArray();

                        intent.putExtra("title", titleItem);
                        intent.putExtra("BMP",bytes);
                        intent.putExtra("desc", descItem);

                        Log.i("ON ITEM CLICK", "OK");
                        startActivity(intent);
                    }
                });
                Looper.loop();
            }
        };
        new Thread(runnable).start();
        stopThread(Thread.currentThread());
    }
});

Can you please tell me if you see something wrong or if there is a better way of doing the same or to optimize it?

@ratchetfreak's comment is correct, expanding on it here a bit:

AsyncTask is the class to use (or rather, extend and use your subclass-implementation, as AsyncTask itself is an abstract class).

You should use AsyncTask for performing operations that otherwise would block the UI-thread and thus would make your application not respond.

However, let's take a look at what your code actually does:

Creates a runnable
Gives the runnable a View, from which it takes a Context, two TextViews (and their Strings), and a Bitmap from an ImageView.
Then it creates and shows a toast - this should be done on the UI-thread and not in a background thread
Posts a runnable to your myHandler, therefore making the following code run on your UI-thread:

Create an intent
Compress a bitmap into JPEG
Puts some data into the intent
Starts a new activity

After the runnable has been created and a new thread started, you call a method that seems to want to stop your current thread!? Why on earth would you want to do that?? Let Android take care of the thread handling! That thread is not yours to stop!

Why do you create these threads?

The longest running operation that you perform in these threads is the Compress a bitmap into JPEG, which you run on the UI-thread anyway. The few things that you do in background is so fast that you really don't need to create a new thread for it.

If your Compress a bitmap into JPEG operation takes much time, then I would suggest you create an AsyncTask<Bitmap, Void, byte[]> to handle that. (Bitmap is the input, it does not have any progress update and therefore it says Void, and it returns a byte[] which you will send to the next activity). I don't think you need any AsyncTask at all here though.

Your new code:

With the above comments taken into consideration, here's the code you need. No extra threads. If this code causes your app to stop respond for a longer while than what is comfortable, then you can consider an AsyncTask.

myList.setOnItemClickListener(new OnItemClickListener() {

    @Override
    public void onItemClick(AdapterView<?> parent, final View view,
            int position, long id) {
        Context context = view.getContext();
        String titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
        String descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
        Bitmap bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();
        Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();

        Intent intent = new Intent(getApplicationContext(), ItemDetails.class);
        ByteArrayOutputStream stream = new ByteArrayOutputStream();
        bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
        byte[] bytes = stream.toByteArray();

        intent.putExtra("title", titleItem);
        intent.putExtra("BMP",bytes);
        intent.putExtra("desc", descItem);
        Log.i("ON ITEM CLICK", "OK");
        startActivity(intent);
    }
});

Normal usage of AsyncTask

Whenever you have a longer running computational-intensive operation, then you can consider an AsyncTask. Also whenever you do network operations (download a file from internet, create a long-running socket-connection, etc.), then you need an AsyncTask. As said already, I don't think your current code needs/benefits from an AsyncTask.

Consider, however...

Your ImageView known as view.findViewById(R.id.imgItem) must have gotten its drawable from somewhere. Is it loaded from a file on the external storage? Is it loaded from a resource? Rather than compressing the drawable's bitmap data and sending the byte array, consider informing your new activity about where it can find the original data. If it's a file on the external storage, pass along a String for the path it is found in. If it's a resource in your application, pass along the resource int identifier.

If the drawable is loaded from the internet or dynamically created, then I agree about compressing it as JPEG and sending the byte array to the new activity.


Type determined return values I have the following js code:

if (typeof errorCallback === "function") {
    var result = errorCallback.apply(this, arguments);
    // if errorCallback returns a non-false value, end execution
    if (result) {
        return result;
    }
}

// if there's no errorCallback or errorCallback returned 'false',
// 'null', 'undefined', '', 0, hell-knows-what-else ...
// note, that we don't return anything here
defaultErrorCallback.apply(this, arguments);

So far I came up with this:

return (typeof errorCallback=== "function"
    ? errorCallback.apply(this, arguments)
    : void 0) || defaultErrorCallback.apply(this, arguments);

But this will return (in some cases) the result of defaultErrorCallback whereas I want it to return undefined.

Can this code be shortened without changing functionality?

More of a golf question than a CodeReview question really,

I would first consider

return (c
  ? someFunc.apply(this, arguments) || undefined
  : undefined)

The interesting part is that if otherFunc is not a function, then you would have executed it as a function anyway..
Furthemore,

I like better someFunc instanceof Function than typeof someFunc === "function"
For something this fancy I would go for the Spartan f
I tend to avoid void except for bookmarklets

so perhaps something like this:

//Unless f is a function and returns a non false value, return undefined
return (f instanceof Function ? f.apply(this, arguments) : undefined )
       || alternativeFunction.apply(this,arguments );


Outputting Fractions This program is to return a number one at a time from a given input list of numbers, with a frequency proportional to probability/fraction associated with each number.

I know it could be made a generator function. But if I have to do it inside a class, can you please suggest improvements to this code, particularly with regards to the exception classes I have made below?

from random import random
from bisect import bisect_right

class Invalid_fraction(ValueError): pass
class Negative_fraction(ValueError): pass
class Sum_fraction(ValueError): pass
class NullValues(ValueError): pass

class RandomGen(object):
    def __init__(self, nums, fractions):
        #Basic error checks
        if ( len(nums)==0 or len(fractions) == 0):
            raise NullValues("Null sample space")

        if ( len(nums) != len(fractions) ):
            raise Invalid_fraction("Invalid fraction list")

        #Data initiallisations
        self._sum  = 0
        self._fractions= []
        self._nums = nums

    #Allocating a range within 0-1 to each of the input number, along with check for negative fraction.

        for p in fractions:
            if p < 0:
                raise Negative_fraction("negative fraction")
            self._sum += p
            self._fractions.append(self._sum)

        if self._sum != 1:
            raise Sum_fraction("Sum of fractions is not equal to 1")

    def next_num(self):
        #Get a random number between 0 and 1 and yield the number of the bucket
        random_number = random()
        index = bisect_right(self._fractions,random_number)
        return self._nums[index]

Frankly, it seems unnecessary to have any custom exceptions (and if you keep them, they should all have PEP-8-compliant names). All of the custom exceptions could just be ValueErrors ("when a built-in operation or function receives an argument that has the right type but an inappropriate value") with different messages, e.g.:

if not nums or not fractions: # note empty collections evaluate False in a boolean context
    raise ValueError("Null sample space")


Keeping the average user from opening two copies of my program I know that this is an impossible task against someone determined, but I'd like to keep mid-average users from being able to open more than one copy of my program at a time.

Here's the quick code I've made:

const tmp = require('tmp'),
    fs = require('fs'),
    path = require('path');

// Randomly generated, constant in each copy of the app
const app_code = '19c2cefbfa76b4887a1d4664704a3fe0';

// Write protection here, although it doesn't help much
const lock = tmp.fileSync({ mode: 0444, prefix: app_code});

const filedir = path.dirname(lock.name);
const filename = path.basename(lock.name);

// Search for already running copies
fs.readdirSync(filedir).forEach(f => {
    // If it's made by the app and it's not mine
    if (f.includes(app_code) && f !== filename) {
        // Delete my lock and close
        fs.unlink(lock.name);
        process.exit(1);
    }
});

// Watch for my own lock being deleted
fs.watch(filedir, function(event, who) {
    if (event === 'rename') {
        // If my lock doesn't exist or a new lock has been made
        if (!fs.existsSync(lock.name) || who.includes(app_code)) {
            // Delete my lock and close
            fs.unlink(lock.name);
            process.exit(1);
        }
    }
});

// Main program content, use placeholder here:
setInterval(() => console.log("Running alone"), 1000);

There are a few issues that I see right off the bat:

Different tmp folders being used
Locks being left in place if the program is terminated, meaning no copies could run

Anything obvious I'm missing?

I suggest you to use resource based locking. Consider the following:

your application uses well known TCP/UDP port. Only single process can bind it.
your application uses some configuration/database/whatever files in predefined places. flock them. Same stands for pid files under /var/run.

It is still possible to separate resources with chroot or containers. But if nothing is shared are there any benefits in forced locking?

  I'd like to keep mid-average users from being able to open more than one copy of my program at a time.

I guess mid-average users won't use network namespaces, chroots or containers. If they will - I bet they have the reasones so let them do the thing.


Verify if user has at least one of required roles to be able to update a record owned by a certain user I have a Microsoft CRM 2016 plugin that checks if the user that triggers an update message has at least one of the required roles, when the updated record is owned by a specific user.

Here's the code:

public class CheckIfOwnedByPool : BasePlugin
{
    public CheckIfOwnedByPool(string unsecure, string secure)
        : base(unsecure, secure)
    {

    }

    public override void ExecutePluginLogic(XrmObjects xrmObjects)
    {

        if (xrmObjects.PluginContext.MessageName.ToLower() == "update" && xrmObjects.PluginContext.PreEntityImages.Contains("entity"))
        {

            Guid PoolId = Guid.Empty;
            List<Guid> allowedRoles = new List<Guid>();
            var entity = xrmObjects.PluginContext.PreEntityImages["entity"];

            //retrieve id of "POOL" team and allowed roles GUIDs from plugin step XML config. GUID varies from environment to environment...
            try
            {
                PoolId = new Guid(GetAllNodes("ownerconfig")[0].Attributes["value"].InnerText);
                GetAllNodes("roles")[0].Attributes["value"].InnerText.Split(';').ToList().ForEach(x => allowedRoles.Add(new Guid(x)));
            }
            catch (Exception ex)
            {
                throw new InvalidPluginExecutionException(string.Format("Error deserializing plugin step XML config: {0}", ex.Message));
            }

            if (PoolId == Guid.Empty || allowedRoles.Count == 0)
                return;

            //if entity is owned by team called "POOL"
            if (((EntityReference)entity["ownerid"]).Id == PoolId)
            {
                using (XrmServiceContext ctx = new XrmServiceContext(xrmObjects.Service))
                {
                    //retrieve current user roles
                    var userRoles = ctx.SystemUserRolesSet.Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId).ToList();

                    //check if current user has one of required roles
                    if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))
                        throw new InvalidPluginExecutionException("You do not have access to modify this record. Please see with your supervisor");

                }

            }

        }
    }
}

This seems to work fine and is registered in pre-operation of Update of any entity. As observed, if one plugin in pre-operation throws an exception, all the plugin stack is stopped, meaning all other pre/post operation plugins won't execute, which is what I want.

Do you see anything that can go wrong? The only issue is that every update done on an entity owned by "POOL", there will be a retrieve to get the user's role. This could hurt performance, but by how much ?

BasePlugin is an abstract class implementing IPlugin and xrmObjects holds useful members such as the tracing service, the plugin exec. context, etc. GetAllNodes is a member of BasePlugin which allows for easy access of the XML plugin step config.

try
{
    PoolId = new Guid(GetAllNodes("ownerconfig")[0].Attributes["value"].InnerText);
    GetAllNodes("roles")[0].Attributes["value"].InnerText.Split(';').ToList().ForEach(x => allowedRoles.Add(new Guid(x)));
}
catch (Exception ex)
{
    throw new InvalidPluginExecutionException(string.Format("Error deserializing plugin step XML config: {0}", ex.Message));
}

This is really bad. You are hiding the original exception. If you don't want to pass it as an inner exception at least do ex.ToString() to be able to see the stack-trace later if necessary.

As a matter of fact this try/catch could be a separate method that gets the PoolId from the xml you could then just return the PoolId without the helper variable outside this try/catch block.

if (xrmObjects.PluginContext.MessageName.ToLower() == "update" && xrmObjects.PluginContext.PreEntityImages.Contains("entity")) {

Make the method return if the condition is false to reduce nesting.

if (PoolId == Guid.Empty || allowedRoles.Count == 0)
   return;

and

if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))
   throw new InvalidPluginExecutionException(...)

I suggest you always use curly braces {}. It can prevent a lot of errors.

if (((EntityReference)entity["ownerid"]).Id == PoolId)

Same here, return if false.

using (XrmServiceContext ctx = new XrmServiceContext(xrmObjects.Service))

and

var userRoles =

Be consistant. Use either var or types everywhere.

var userRoles = ctx.SystemUserRolesSet.Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId).ToList();

//check if current user has one of required roles
if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))

You can merge it into:

var userHasRequiredRole =
    ctx.SystemUserRolesSet
    .Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId)
    .Any(x => allowedRoles.Any(y => y == x.RoleId.Value));

so you can remove the comment and simply write:

if (!userHasRequiredRole) { throw... }

or even shorter:

var userHasRequiredRole =
    ctx.SystemUserRolesSet
    .Any(x =>
        x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId &&
        allowedRoles.Any(y => y == x.RoleId.Value));

although personally I'd use the Where+Any, it looks cleaner


Finding recessions in US GDP data using pandas For an assignment, I am identifying the first quarter of the 2008 recession in the United States. The Excel data I'm using can be downloaded here: gdplev.xls. How can I improve this pandas code to make it more idiomatic or optimized?

def get_recession_start():
    '''Returns the year and quarter of the recession start time as a
    string value in a format such as 2005q3'''
    GDP_df = pd.read_excel("gdplev.xls",
                           names=["Quarter", "GDP in 2009 dollars"],
                           parse_cols = "E,G",
                           skiprows = 7)
    GDP_df = GDP_df.query("Quarter >= '2000q1'")
    GDP_df["Growth"] = GDP_df["GDP in 2009 dollars"].pct_change()
    GDP_df = GDP_df.reset_index(drop=True)
    # recession defined as two consecutive quarters of negative growth
    GDP_df["Recession"] = (GDP_df.Growth < 0) & (GDP_df.Growth.shift(-1) < 0)
    return GDP_df.iloc[GDP_df["Recession"].idxmax()]["Quarter"]
get_recession_start()

Your function does too many things: reading Excel file, filtering necessary rows, and calculating the "recession_start". My advice is to take the first two out.

Also, supply quarters and GDP as separate objects (pd.Series) to the function instead of the DataFrame. Like this you will remove hardcoded strings from your function, and, what's more important, you will get rid of SettingWithCopyWarning that you should get right now:

df = pd.read_excel('gdplev.xls',
                   names=['Quarter', 'GDP in 2009 dollars'],
                   usecols='E,G',
                   skiprows=7)
mask = df['Quarter'] >= '2000q1'
print(get_recession_start(quarters=df.loc[mask, 'Quarter'],
                          gdps=df.loc[mask, 'GDP in 2009 dollars']))

Note that I use usecols instead of parse_cols as it is deprecated. Also, I removed df.query in favor of boolean masking and .loc.

Then, the function would look like this:

def get_recession_start(quarters: pd.Series,
                        gdps: pd.Series) -> str:
    """
    Returns the year and quarter of the recession start time
    as a string value in a format such as 2005q3
    """
    growth = gdps.pct_change()
    recession = (growth < 0) & (growth.shift(-1) < 0)
    recession = recession.reset_index(drop=True)
    return quarters.iloc[recession.idxmax()]

Here I also used triple double quotes for the docstring and type hints. IMHO, this looks much cleaner.

Probably, it would also make sense to return only the recession.idxmax() index and get corresponding quarters value outside of the function.


Console application for providing detailed error messages I'm writing a console application where I need to provide detailed error messages in case anything goes wrong. As a result of this, I find myself splitting up my code into many pieces and making sure that every step is handled correctly. While this certainly makes it possible to detail what exactly went wrong, it unfortunately also leads some horrific overhead on my code. Take the following code for example:

        // Open the SISS catalog on the server
        Catalog catalog = integrationServices.Catalogs[catalogName];
        if (catalog == null)
        {
            Console.WriteLine("Unable to open the SSIS catalog : " + catalogName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Open the folder in the catalog
        CatalogFolder folder = catalog.Folders[folderName];
        if (folder == null)
        {
            Console.WriteLine("Unable to open the folder : " + folderName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Open the project in the folder
        ProjectInfo projectInfo = folder.Projects[projectName];
        if (projectInfo == null)
        {
            Console.WriteLine("Unable to open the project : " + projectName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Check if the package exists
        if (!projectInfo.Packages.Contains(packageName))
        {
            Console.WriteLine("Unable to open the package : " + packageName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

Is there any way I can make this more pretty?

Make a function that writes an error message (String) to the console, then exits with failure error code. But I'm not sure whether this would mess up your returns and overall control flow.

Other than that, I don't know. Yes, you can golf the string to "Unable to open X : " + x + ", it does not exist on the server", but all you're doing is making the error message harder to find when debugging later.

That said, "pretty" is pretty (Hah) abstract. As it is now, the code is perfectly readable, and it doesn't contain too much duplication. It doesn't contain any performance issues either. What you're seeing here is your code reaching the lowest Kolmogorov Complexity it can.

But let's say, for the heck of it, you were to create some sort of function that takes a catalogName and returns either null or a Catalog object. In case of null, you have to call another function to get the error message. If you did that, you're just relocating the if statements. It also makes the design confusing. Basically, I don't think you can because you have to check and you have to report. In my opinion, you did that the prettiest way possible.


Audio processing with normalized [0.0, 1.0] domain I've a code that process buffers of samples/audio data. Here's the code:

#include <iostream>
#include <math.h>
#include <cstring>
#include <algorithm>

#define PI 3.141592653589793238
#define TWOPI 6.283185307179586476

const int blockSize = 256;
const double mSampleRate = 44100.0;
const double mHostPitch = 2.0;
const double mRadiansPerSample = TWOPI / mSampleRate;

double mGainNormalizedValues[blockSize];
double mPitchNormalizedValues[blockSize];
double mOffsetNormalizedValues[blockSize];

class Oscillator
{
public:
    double mPhase = 0.0;
    double minPitch = -48.0;
    double maxPitch = 48.0;
    double rangePitch = maxPitch - minPitch;
    double pitchPd = log(2.0) / 12.0;
    double minOffset = -900.0;
    double maxOffset = 900.0;
    double rangeOffset = maxOffset - minOffset;

    Oscillator() { }

    void ProcessBuffer(double voiceFrequency, int blockSize, double *left, double *right) {
        // precomputed data
        double bp0 = voiceFrequency * mHostPitch;

        // process block
        for (int sampleIndex = 0; sampleIndex < blockSize; sampleIndex++) {
            double output = (sin(mPhase)) * mGainNormalizedValues[sampleIndex];

            *left++ += output;
            *right++ += output;

            // next phase
            mPhase += std::clamp(mRadiansPerSample * (bp0 * WarpPitch(mPitchNormalizedValues[sampleIndex])) + WarpOffset(mOffsetNormalizedValues[sampleIndex]), 0.0, PI);
            while (mPhase >= TWOPI) { mPhase -= TWOPI; }
        }
    }

    inline double WarpPitch(double normalizedValue) { return exp((minPitch + normalizedValue * rangePitch) * pitchPd); }
    inline double WarpOffset(double normalizedValue) { return minOffset + normalizedValue * rangeOffset; }
};

int main(int argc, const char *argv[]) {
    int numBuffer = 1024;
    int counterBuffer = 0;

    Oscillator oscillator;

    // I fill the buffer often
    while (counterBuffer < numBuffer) {
        // init buffer
        double bufferLeft[blockSize];
        double bufferRight[blockSize];
        memset(&bufferLeft, 0, blockSize * sizeof(double));
        memset(&bufferRight, 0, blockSize * sizeof(double));

        // emulate params values for this buffer
        for(int i = 0; i < blockSize; i++) {
            mGainNormalizedValues[i] = i / (double)blockSize;
            mPitchNormalizedValues[i] = i / (double)blockSize;
            mOffsetNormalizedValues[i] = i / (double)blockSize;
        }

        // process osc buffer
        oscillator.ProcessBuffer(130.81278, blockSize, &bufferLeft[0], &bufferRight[0]);

        // do somethings with buffer

        counterBuffer++;
    }
}

Basically:

init a Oscillator object
for each buffer, fill some param arrays with values (gain, pitch, offset); gain remain normalized [0.0, 1.0], while pitch and offset range in -48/48 and -900/900
then I iterate the buffer, calculating the Oscillator's sine due to pitch and offset, and I apply a gain; later, I move the phase, incrementing it

The whole domain of operations are normalized [0.0, 1.0]. But when I need to manage pitch and offset, I need to switch domain and use different values (i.e. the Warp functions).

This required lots of computations and process. I'd like to avoid it, so I can improve the code and performances.

How would you do it? Can I keep into [0.0, 1.0]? Could I improve the performance?

Prefer C++ headers

Instead of <math.h>, it's better to include <cmath> and qualify names such as std::log.

Prefer constants to macros

Re-write pi as a strongly-typed, scoped variable rather than a preprocessor macro.  Same for 2*pi if you really feel the need.

Manage line lengths

Some lines are very long.  In many cases, they just need newlines adding (e.g. bodies of inline functions can have their own lines).

In the case of the std::clamp() call, it's probably worth using variables to give a name to the candidate value before clamping.

Easier sizeof

Instead of recomputing the size of bufferLeft and bufferRight like this:

    memset(&bufferLeft, 0, blockSize * sizeof(double));
    memset(&bufferRight, 0, blockSize * sizeof(double));

It's easier and clearer to just use the whole array size:

    memset(&bufferLeft, 0, sizeof bufferLeft);
    memset(&bufferRight, 0, sizeof bufferRight);

Personally, I'd generally prefer std::fill to match types and ensure the intention is clear:

    std::fill(std::begin(bufferLeft), std::end(bufferLeft), 0.0);
    std::fill(std::begin(bufferRight), std::end(bufferRight), 0.0);

Then I don't need to think about whether all-bits zero is the same as 0.0 or not.


Bash function to create the next SVN release tag I've created the below script and would appreciate any feedback.

# svn directory ^/tags/release/
# 1/
# 2/
# 3/
# 4/
# usage: svnnextrelease fixes email formatting
# generated line: svn cp ^/trunk ^/tags/release/# -m'fixes email formatting'

function svnnextrelease(){
NextVersion=$(svn ls ^/tags/release | tail -n 1 | while read tag; do expr $(echo "${tag%?}") + 1; done)
# svn ls ^/tags/release, listing the directories
# tail -n 1, give me the last (latest) directory
# while loop reads result
# expr $(echo "${tag%?}") + 1, echo statment removes the "/" so 4/ becomes 4. expr then increments it.
# echo $NextVersion; 5

svn cp ^/trunk ^/tags/release/$NextVersion -m'$@'
}

Here is a small improvement.

svnnextrelease() {
  v=$(svn ls ^/tags/release | tail -n 1)
  svn cp ^/trunk ^/tags/release/$((${v%/} + 1 )) -m "$@"
}

You can use $(( )) as a replacement for expr in bash. (depends on your taste.)
bash can cheaply get rid of / for you.


Creating a string from command line arguments I am fairly new to C and am just overcoming my fear of pointers and segmentation faults. C is a strange language to me because I come from more object oriented languages like JavaScript and Java.

When I code in C, everything feels all "one-liney" to me. I'm used to things, for example in Java, being neatly spread out, everything with it's own clean section in the code.

When I see C(ha) that I write, everything is all in one place and nothing has it's own area.

Here is an example piece of code that I recently wrote. It takes all the command line arguments and puts them all into one string with each word separated by a space (dismiss any and all spelling errors):

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
    if(argc == 1) { // if there are too few arguments
        printf("ERROR: Expected atleast 1 argument\n");
        return 0;
    }
    int size = 0;
    while(argv[size]) { // gets the amount of arguments besides the first argument
        size++;
    }
    size--;
    if(size == 1) { // if there is only 1 word
        printf("%s\n", argv[1]);
        return 0;
    }
    int i;
    int v = strlen(argv[1]) + strlen(argv[2]); // for allocating memroy

    char *str = (char *)malloc(v);
    strcat(str, argv[1]); // putting the string together
    strcat(str, " ");
    strcat(str, argv[2]);
    strcat(str, " ");
    for(i = 3; i <= size; i++) {
        str = (char *)realloc(str, (v + strlen(argv[i]))); // reallocates enough memory
        strcat(str, argv[i]);
        strcat(str, " ");
    }
    printf("%s\n", str);
    return 0;
}

One thing that I read somewhere about C involving convention is that C should be broken up into smaller bits of code (functions?). Is this true, and what else should I work on?

In the last lines of your question, what you mention is C being a procedural language. This means, you should split your semantic logic into reusable smaller logical bits and implement smaller portions of your code and implement your program by using the functions you implemented.

Although, as you come from a object-oriented language, I believe you must be familiar with the concepts I mentioned.

Coming to your example, your program consists of a fairly small codebase and it does these things:

Iterate through the argv strings
Combine the argv strings in another string

But if I did not misunderstand, your first iteration is to find the count of the arguments. That is what argc variable, therefore your first while loop is unnecessary, you can simply set size = argc - 1;

You have an unnecessary condition check and this part is unnecessary:

strcat(str, argv[1]); // putting the string together
strcat(str, " ");
strcat(str, argv[2]);
strcat(str, " ");

You should start your for loop from 1 instead of 3, and remove these lines.

Your condition check also seems unnecessary, as your for for loop would produce similar results.

Therefore, I would not say that this code is bad due to not being written in procedural paradigm, but it is bad due to overall unnecessary lines of code.

A shorter approach would be:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
    if(argc == 1) {
        printf("ERROR: Expected at least 1 argument\n");
        return 0;
    }

    int i, v = 0, size = argc - 1;

    char *str = (char *)malloc(v);

    for(i = 1; i <= size; i++) {
        str = (char *)realloc(str, (v + strlen(argv[i])));
        strcat(str, argv[i]);
        strcat(str, " ");
    }

    printf("%s\n", str);
    return 0;
}