AGGRCOW on SPOJ – Maximize the minimum distance between aggressive cows I am getting a TLE on my submission to AGGRCOW - Aggressive cows on SPOJ:

  Farmer John has built a new long barn, with N (2 <= N <= 100,000)
  stalls. The stalls are located along a straight line at positions
  x1,...,xN (0 <= xi <= 1,000,000,000).

  His C (2 <= C <= N) cows don't like this barn layout and become
  aggressive towards each other once put into a stall. To prevent the
  cows from hurting each other, FJ wants to assign the cows to the
  stalls, such that the minimum distance between any two of them is as
  large as possible. What is the largest minimum distance?

  Input

  t – the number of test cases, then t test cases follows.
  * Line 1: Two space-separated integers: N and C
  * Lines 2..N+1: Line i+1 contains an integer stall location, xi

  Output

  For each test case output one integer: the largest minimum distance.

  Example

  Input:

1
5 3
1
2
8
4
9

  Output:

3

  Output details:

  FJ can put his 3 cows in the stalls at positions 1, 4 and 8,
  resulting in a minimum distance of 3.

I tried checking starting from smallest minimum distance i.e zero to the largest minimum distance that will be the solution. I used recursion to solve this question but getting TLE. How can I use binary search in this problem for making it more efficient.

#include<bits/stdc++.h>
using namespace std;

bool CanBeMin( vector<long long int> arr, unsigned int b,int s,unsigned int
     cnt,long long int k){
           if(cnt == b ) return true;
           if(arr[s+1]=='\0') return false;
           for(unsigned int i=s+1 ; arr[i]!='\0'; i++){
                 if (arr[i] - arr[s] > k && b-cnt <= arr.size() - i) {
                            cnt ++;
                            return CanBeMin ( arr , b , i , cnt , k);}
                 }
                 return false;}

int count ( vector <long long int> arr, unsigned int b){
      long long int k=0;
      while (CanBeMin ( arr , b , 0 , 0 , k)) {
              k=k+1;}
      return k;}

int main(){
     int t;
     cin >> t;
     while ( t-- ){
         int a , b;
         cin >> a >> b;
         vector<long long int> arr(a);
             for( int i = 0; i < a; i++) {
                 cin >> arr[i];}
         sort ( arr.begin(), arr.end());
         if (b <= a)
         cout << count (arr, b-1);
         else cout<<"0";
         cout << endl; }
       }

Well, your indentation gives me a headache. I think there might be a pattern in it, but I don't see it, so I'll reformat it for writing my answer.
<bits/stdc++.h> is non-standard and causes longer compile-times. Maybe just include what you need?
Never use using namespace something_humungeous_and_growing_like_std;, because there might be conflicts, and silent changes of semantics.
There are single-character-names which would actually make sense for your variables: The ones used in the spec. If you don't want to use them, put a bit more effort into finding meaningful names.
You never test whether input succeeds. At the very least, add the following to get an exception (though it messes with the input-processing ordinarily used in bigger / more robust programs):

std::cin.exceptions(std::ios_base::failbit);

Avoid flushing streams (use '\n' instead of std::endl) unless you actually need to flush.
Prefer streaming character literals to size-1-strings. It's potentially marginally more efficient.
Decouple iostreams from stdio.
Refrain from copying expensive things. Every single time you pass your vector by value Needs a copy, thus a useless extra-allocation, -deallocation and block-copy.
Also, while it is generally a good idea to minimize the scope of variables, re-using the same vector over multiple iterations allows you to forego extra-allocations and de-allocations.
You are using extremely dumb brute-force in count() by trying minimal distances starting at zero and then counting up.
Consider using binary-search instead, trivial minimum is \$0\$, maximum is \$(maxpos - minpos) / (cows - 1)\$.
Same in CanBeMin(), use binary-search. std::lower_bound() can help.


Sort array into wave like array I am solving interview questions from here.

  Problem : Given an array of integers, sort the array into a wave like array and return it, in other words, arrange the elements into a
  sequence such that a1 >= a2 <= a3 >= a4 <= a5.

  Note - If there are multiple answers possible than return the one that is lexicographically smallest.

  Approach : First sort the array then swap every second number with previous number.

This is my solution:

def wave(array):
    """Returns array in wave pattern by first sorting
    the array and then swapping the adjacent digits"""

    array.sort()  ##sort the given array
    for i in range(0,(len(array)-1),2):  ##skip every second number
        array[i], array[i+1] = array[i+1], array[i]  ##swap adjacent numbers

    return array

print wave([6,7,8,1,2,3,4])
print wave([5,1,3,4,2])
print wave([11,3,5,2,7,1,6,8,10,9,4])

How can i make this code better? Is there any other approach to solve this problem?

Because of the lexicographically smallest tie breaker restriction, you're basically funelled down this approach. In particular, you can't do without the sort.

"""Returns array in wave pattern by first sorting
the array and then swapping the adjacent digits"""

Docstrings are interface level documentation. They should describe what a function does, not how it does it. For a description of how a given implementation works, use comments.

array.sort()  ##sort the given array

Consider whether it's appropriate to do this in place (and so mangle the input array) or whether it would be better to use sorted

for i in range(0,(len(array)-1),2):  ##skip every second number
    array[i], array[i+1] = array[i+1], array[i]  ##swap adjacent numbers

You could rewrite this generators or using various tools from itertools if you wanted to avoid the explicit for loop.

However, what you have is clearer about what's going on and, given that you are working from a list already, reasonable in terms of performance.

print wave([6,7,8,1,2,3,4])
print wave([5,1,3,4,2])
print wave([11,3,5,2,7,1,6,8,10,9,4])

I note that you're only checking odd length lists with no negatives and no duplicates. Consider more comprehensive testing.


Comma-formatted STL vectors I move back and forth between Python and C++ and I often need a nice/quick way to output STL objects to the screen for debugging purposes. I'd like the output to match the output of a comparable Python object, thus I have different templates for vectors, sets, etc...

Is this the best way to go about this?

template <class T>
ostream& operator<<(ostream& s, const vector<T> &A) {
  if(A.empty())
    return s << "[]";

  s << "[";
  typename vector<T>::const_iterator itr_penultimate = --A.end();
  typename vector<T>::const_iterator itr = A.begin();

  while(itr != itr_penultimate) {
    s << *itr << ", ";
    itr++;
  }
  return s << *itr << "]";
}

The above answer is fine.

The only problem I have is the extra ',' in the output:

std::cout << "[ ";
std::copy(data.begin(), data.end(), std::ostream_iterator<int>(std::cout, ", "));
std::cout << " ]\n";

Generates the output:

[ 1, 2, 3, 4, 5, 6,  ]
                  ^  extra comma

You can get around this with some extra work.

auto end = data.end();
if (!data.empty()) { --end;}
std::cout << "[ ";
std::copy(data.begin(), end, std::ostream_iterator<int>(std::cout, ", "));
if (!data.empty()) {std::cout << *end;}
std::cout << " ]\n";

Generates the output:

[ 1, 2, 3, 4, 5, 6 ]

This is better but it sort of defeats the purpose of using algorithms.

If we had used the loop it would look like this:

std::cout << "[";
for(auto loop = data.begin(); loop != data.end(); ++loop)
{
     std::cout << *itr << ", ";
}
std::cout << "]";

Of course this has the same problem as the first version of the algorithm above. So if we take that into account you can re-write like this:

std::cout << "[";
auto begin = data.begin();
if (!data.empty) {std::cout << *begin;++begin}
for(auto loop = begin; loop != data.end(); ++loop)
{
     std::cout << ", " << *loop;
}
std::cout << "]";

Now it works. And because it only use one test on empty is better than the altered algorithm version in my opinion. So what we really need is a version of the output iterator that does the above.

template<typename T>
class PrefexOutputIterator
{
    std::ostream&       ostream;
    std::string         prefix;
    bool                first;
    public:

    typedef std::size_t                 difference_type;
    typedef T                           value_type;
    typedef T*                          pointer;
    typedef T                           reference;
    typedef std::output_iterator_tag    iterator_category;

        PrefexOutputIterator(std::ostream& o,std::string const& p = ""): ostream(o), prefix(p), first(true) {}

        PrefexOutputIterator& operator*()       {return *this;}
        PrefexOutputIterator& operator++()      {return *this;}
        PrefexOutputIterator& operator++(int)   {return *this;}

        void operator=(T const& value)
        {
            if (first)      {ostream << value;first = false;}
            else            {ostream << prefix << value;}
        }
};

Now we can use this and get the output we want:

std::cout << "[";
std::copy(data.begin(), data.end(), PrefexOutputIterator<int>(std::cout, ", "));
std::cout << "]";

The output is:

[1, 2, 3, 4, 5, 6]

Nowadays though I seem to be using a lot of Json.
So now I use this template librrys: https://github.com/Loki-Astari/ThorsSerializer

using ThorsAnvil::Serialize::jsonExport;
using ThorsAnvil::Serialize::jsonImport;

std::cout << jsonExport(data) << "\n"; // Serialize the array as json (looks like above)
std::cin  >> jsonImport(data);         // Reads a json array into an array.


Compare elements of each list of lists to corresponding ones in different list My goal is to reduce processing time significantly (if possible) by making this working code more efficient. Currently 50k row by 105 column data taking about overall 2 hours to process. Share of this piece is 95%.

This piece is a key part of my Python 3.6.3 script that compares two set of list of lists element by element regardless of datatype. I spent long hours but seems I reached my limits here. This is running on Windows 10.

Sorry about lots of variables. Here is description:
Ap, Bu - list of lists. Each sublist

may contain any datatype (usually String, Number, Null, Date).
1st element of list within list is always unique string.
has equal number of elements as other lists
each list in Ap has corresponding list in Bu (corresponding in here means 1st element and ID element (not necessarily other elements) of a sublist  of Ap matches that of Bu, that's considered there is corresponding match)
each sublist will contain unique ID in the same position. Meaning index of ID is same in every sublist (be it Ap or Bu).

prx - is index of a list within Ap

urx - corresponding/matching index of a list within Bu, as evidenced by urx=l_urx.index (prx)

cx - is index of an element in a single list of Au
ux - is a corresponding element index of an element in a matching list of Bu,
     as evidenced by ux = l_ux.index(cx)

rng_lenAp  - is range(len(Ap))
rng_ls - is range(individual list within Ap)

To visualize (just example):

Ap = [['egg', 12/12/2000, 10, ID1, NULL], ['goog', 23, 100, ID2,12/12/2000]]
Bu = [['goog', '3434', 100, ID2, 12/12/2000], ['egg', 12/12/2000, 45, ID1, NULL]]

for prx in rng_lenAp:
    urx = l_urx.index (prx)
    if Ap[prx][0] == Bu[urx][0]:
        for cx in rng_ls:
            ux = l_ux.index(cx)
            #If not header, non-matching cells get recorded with their current value
            if cx!=0 and Ap[prx][cx] != Bu[urx][ux]:
                output[prx].append (str(Ap[prx][cx] + '^' + str(Bu[urx][ux]))
            #Unless it is row header or ID in column, matching cells gets 'ok'
            elif cx!=0 and prx!=0 and urx !=0 and Ap[prx][cx] == Bu[urx][ux]:
                output[prx].append ('ok' +'^' + 'ok')
            # Anything else gets recorded with their current value
            else:
                output[prx].append (str(Ap[prx][cx] + '^' + str(Bu[urx][ux]))

There must a way to reduce processing time drastically. Currently the cell by cell comparison of 50k row by 100 column data to 50k row by 100 column data is takes about 2 hours. Expected under 30 min. 3.1 GHz, 4 CPUs (8196MB RAM).

Your algorithm uses the list.index function to get the indices of the sublists which is an \$O(n)\$ operation. You are also iterating through all the rows and columns of the data which is an \$O(k \times m)\$ operation where \$k\$ is the number of rows and \$m\$ is the number of columns. In the end it is an \$O(n \times k \times m)\$ algorithm.

In my algorithm getting the intersection of the keys is an \$O(\min(\text{len}(A), \text{len}(B)))\$ complexity then iterating the sublists is an \$O(m)\$ which results in an \$O(\min(\text{len}(A), \text{len}(B)) \times m)\$ complexity.

My alternate solution is based on the fact that the first element of each sublist is unique.
I would structure the data in a different way and use dictionaries instead of lists.
As each first element is unique this would be the key and the rest of the sublist would be the value in the dictionary.
After inserting all the data in the dicts, taking the intersection of the keys would result in a (hopefully) smaller set of data.
The output would be also a dictionary created while iterating through the resulting keys.

output = defaultdict(list) # from collections
keys = A.keys() & B.keys() # intersect the keys
for key in keys:
    # create output based on sublists
    output[key] = ['ok^ok' if a==b else '{}^{}'.format(a,b) for a,b in zip(A[key],B[key])]

For execution time I generated random strings for keys and values. Each key and sublist element was a randomly generated unique string of length 10. Each key had 50% chance to be included in both A and B and each sublist element also had a 50% to be the same for the corresponding element in the other dictionary. I created 50k keys and 104 sublist element for each key.

Generating the data took me around 26 seconds and creating the output took less than 1 second. The results depend on the matching key ratio of the two datasets but it never took more than 2 seconds to finish.


Genetic algorithm final stage I have written the code for a genetic algorithm and have the crossover function, mutation function, parent selector function and a function to transfer genes. Now I want to put it all together and would like to know if the following code is good programming practice or not.

Species Parents[popSize];
Species Children[popSize];

for(int gen = 0 ; gen < 100 ; gen++){
for(int i = 0; i < popSize ; i ++)
{
    int parentA = chooseParent(Parents);
    int parentB = chooseParent(Parents);
    crossOver(Parents[parentA] , Parent[ParentB] , Children[i]);
    Children[i].mutate();
}
for(int i = 0; i < popSize ; i ++)
{
    transfereGenes(Children[i], Parents[i]);
}
}

Style

Species Parents[popSize];
Species Children[popSize];

It's more common for variables to start with lower case letters.

Species parents[popSize];
Species children[popSize];

That way you can easily tell that Species is a type while parents is a variable.  As it is, I have no way to tell why Parents is capitalized and parentA, etc. are not.

for(int gen = 0 ; gen < 100 ; gen++){
for(int i = 0; i < popSize ; i ++)
{

You're mixing two styles here.  Both are valid, but it's generally preferred to pick one and use it consistently throughout your code.  More of the code is using { on a separate line, so it may be best to standardize on that.  Personally, I prefer it the other way, but as I said, both are valid.

for (int gen = 0; gen < 100; ++gen)
{
    for (int i = 0; i < popSize; ++i)
    {

Also, you indent inside the inner loops but not the outer.  It's unclear if this was intentional or a copy/paste error.  Note that if you indented the outer lines extra to make them appear in a code block, that it's often better to select the whole code and use Ctrl+K to indent them.  That will increase the indent of the entire code, not just the outermost lines.

I also changed the spacing to a more common form.

Preincrement may be defined more efficiently than postincrement.  It's a tiny difference, but some people prefer always using preincrement when either would work functionally.  Note that a compiler may compile out the difference entirely anyway.

< versus !=

A comment argued that it is more common to use != rather than <, but I think that they missed the point.  C++ favors using iterators in this situation.  Since many iterators don't have a meaningful < operator, they use something that looks sort of like

for (auto i = iterator.begin(); i != iterator.end(); iterator++)

However, that doesn't apply if your for loop is over an int.  Because int values do have meaningful <, these are more robust.  There's no need to maintain consistency at the price of robustness.  Consider

for (int i = 0; i != 5; i +=2)

This code will crash with an integer overflow (or loop forever) because i will never equal 5.  And this code might otherwise be a sensible replacement for

for (int i = 0; i < 5; ++i)
{
    if (i % 2 == 0)

Note that if you were using std::array or std::vector and C++11, you could use the new range-based for loop.

for (auto child : Children)

Then you don't have to manually manage your iteration at all.

Missing code

You use Parents[i] but you never set it to anything.  Perhaps chooseParent or crossOver does that for you, but it's not clear.  It's also not clear why you transfereGenes (should that be transfer rather than transfere?) with Children[i] and Parents[i].  Why are those two related now when they weren't earlier?

This is probably also a violation of the Single Responsibility Principle.  Initialization of the Parents and Children arrays presumably occurs somewhere.  But since it doesn't happen here or in a method called something like initialization, it's not obvious where.  Nor is it obviously correct.  We're essentially relying on the function implementations being correct.  But I don't even know what they are supposed to do.  I can guess a bit from the names, but this code won't work at all unless one of them handles initialization.

Is your code correct?  Without knowing what the code it calls does, I have no way of knowing.  If you want more of a review, you may want to post a new question with more context.  Preferably with runnable code.

    crossOver(Parents[parentA] , Parent[ParentB] , Children[i]);

Is this a typo?  Should it be

    crossOver(Parents[parentA], Parents[ParentB], Children[i]);

I can't be sure.  Maybe the missing code has a Parent array as well.  And of course if not, a runnable version would find that trivially with a compile.


WeakReference in AsyncTask I want to use the WeakReference approach to not leak memory with my AsyncTask. I found examples online and on Stackoverflow, but they only get the reference in onPostExecute and I am unsure how to correctly use it in all 3 of the UI methods.
My current approach is like this, but I have no clue if I can get rid of some redundancy. Why can't I just call activityReference.get() in the constructor and then only check for null in each of the UI methods? Why do the examples online call get right before using the WeakReference?

private static class ExampleAsyncTask extends AsyncTask<Integer, Integer, String> {
    private WeakReference<MainActivity> activityReference;

    ExampleAsyncTask(MainActivity context) {
        activityReference = new WeakReference<>(context);
    }

    @Override
    protected void onPreExecute() {
        super.onPreExecute();

        MainActivity activity = activityReference.get();
        if (activity == null || activity.isFinishing()) {
            return;
        }

        activity.progressBar.setVisibility(View.VISIBLE);
    }

    @Override
    protected String doInBackground(Integer... integers) {
        for (int i = 1; i < integers[0]; i++) {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

            publishProgress((i * 100) / integers[0]);
        }

        return "Finished";
    }

    @Override
    protected void onProgressUpdate(Integer... values) {
        super.onProgressUpdate(values);

        MainActivity activity = activityReference.get();
        if (activity == null || activity.isFinishing()) {
            return;
        }

        activity.progressBar.setProgress(values[0]);
    }

    @Override
    protected void onPostExecute(String s) {
        super.onPostExecute(s);

        MainActivity activity = activityReference.get();
        if (activity == null || activity.isFinishing()) {
            return;
        }

        activity.progressBar.setProgress(0);
        activity.progressBar.setVisibility(View.INVISIBLE);
        Toast.makeText(activity, s, Toast.LENGTH_SHORT).show();
    }
}

Check for null in WeakReference everytime you need to use it is necessary because the GC process can clear the instance anytime during your work. Besides, onPreExecute, onPostExecute and doInBackground is run in the different thread, there is also some delay when changing thread and the instance may be cleared in between too.

I have a helper method that does the check for null in WeakReference and its instance. Use it if you like:

public <T> T getInstance(WeakReference<T> weakReference) {
    if (weakReference == null || weakReference.get() == null)
        return null;

    // Do more check for your activity if need

    return weakReference.get();
}


Time spend in scoped timer I want to check some timing of my (pre-C11) C++ and wrote a class:

class ScopedTimer {
    timespec start_,end_;
    std::string name_;      /// name of this timer (used in output)
    bool active_;           /// if not active, no output is printed
    clockid_t clock_id_;    /// clock type used for this timer

public:
    ScopedTimer(std::string name, clockid_t clock_id = CLOCK_REALTIME, bool active = true):
        name_(name),
        active_(active),
        clock_id_(clock_id)
    {
        clock_gettime(clock_id_, &start_);
    }

    ~ScopedTimer(){
        if (active_){
            clock_gettime(clock_id_, &end_);

            double dt_ms = (end_.tv_sec - start_.tv_sec) * 1000.0 + (end_.tv_nsec - start_.tv_nsec) / 1000000.0;

             cout << "runtime " << name_ << "  " << dt_ms  << "  ms" << endl;
        }
    }

};

It is rather easy to check the running time of a full function:

void foo()
{
 ScopedTimer t("foo");
  do_stuff();
}

or also to check a part of a function:

void findCorners(cv::Mat& img, bool with_timing_output)
{
  cv::Mat gx_dil;
  cv::Mat dil_element_x = createDilElement(1);
    {
     ScopedTimer t("dilation during findCorners",with_timing_output);
     cv::dilate(gx_img, gx_dil, dil_element_x, Point(-1, -1), 5, BORDER_DEFAULT);
    }
}

What do you think about it?

It looks fine to me and it does the job. I would only consider changing a couple things:

Decouple it from the platform dependent clock_gettime. This piece of code could be useful on any platform that supports time queries with millisecond precision (basically anything nowadays). If you encapsulate that aspect a little more, it can be easily ported to say, Windows using the QueryPerformance*() WinAPI functions. That clockid_t parameter in the constructor is the only bit leaking a platform specific object to the user, which you can easily remove or replace by a portable typedef/constant.
Support a generic output stream instead of hardcoding std::cout. Also very simple to do, just add an extra std::ostream& member an take an extra parameter in the constructor, which can be defaulted to std::cout.
Unless you want the stream to be flushed after printing a timing, replace endl with a \n. Flushing the stream might have performance implication if you are doing a lot of instrumentation in the code.
Not of consequence in this case, since this class is meant to be a stack-lived variable, however, I'll note it anyways as a general guideline: Avoid declaring booleans and variables smaller than the native word size in the middle of a class. e.g.:

timespec start_,end_;
std::string name_;
bool active_;        // <--- Not the best place for it
clockid_t clock_id_;

On most compilers/platforms, a boolean is the size of a byte, which in that case will introduce padding if declared in the middle of other member variables of different sizes. A good rule of thumb is to always place the small fields, like bools, chars and the like at the very end of the member data declarations, so any padding added by the compiler stays minimal and at the end. Again, not of importance in you code, just thought it would be interesting to mention for future reference.


JS form verification I have three checboxes in my form. I have to check which one is checked and start the action. What do you think? Is this the easiest way of doing it?

Checkbox 1 = cat
Checkbox 2 = dog
Checkbox 3 = dog
Only cat ->           form action
Cat & any of dogs ->  play music
Dog & dog ->          play music
Nothing ->            form action

HTML:

    <form action="sample_action" method="get">
            <input type="hidden" name="sample_name" value="sample_value">

<input id="checkbox1" type="checkbox" name="pet" value="cat">Cat

<input id="checkbox2" type="checkbox" name="pet" value="dog">Dog 1

<input id="checkbox3" type="checkbox" name="pet" value="dog">Dog 2

          <button type="submit">Check my pets</button>
   </form>

Solution:

function checkPets () {
  var cat = $('#checkbox1').is(":checked");
  var dog1 = $('#checkbox2').is(":checked");
  var dog2 = $('#checkbox3').is(":checked");
  return !cat &&(dog1 || dog2);
}

$(window).load(function(){
$('.pets form').submit( function(e){
    if(checkPets()) {
        e.preventDefault();
        $('body').append('<audio></audio>');
        $('audio').attr({'src': 'audio.mp3', 'autoplay': 'autoplay'});
    }
});

Is this the easiest way of doing it?

In my view, yes: I don't see how to dramatically simplify your code.
It only can be (very) slightly reduced using return false; instead of e.preventDefault;.

You also might use the more common $(document).ready() in place of $(window).load().
BTW it'd be more readable to rename your checkPets() function to something like mustPlayMusic(), which reflects that it returns a boolean.

So it results in:

$(document).ready(function(){
  $('.pets form').submit( function(e){
    if(mustPlayMusic()) {
      $('body').append('<audio></audio>');
      $('audio').attr({'src': 'audio.mp3', 'autoplay': 'autoplay'});
      return false;
    }
});

On the other hand, something puzzles me: the checkPets()||mustPlayMusic() function doesn't match the conditions you detailed above.

It should be:

function checkPets () {
  var cat = $('#checkbox1').is(":checked");
  var dog1 = $('#checkbox2').is(":checked");
  var dog2 = $('#checkbox3').is(":checked");
  return (cat && (dog1 || dog2)) || (dog1 && dog2);
}


Promise.spread "Polyfill" I have written this "Polyfill" for Chrome's Promise object and I have some concerns about the implementation;

Does it actually work as designed? It seems to work but I'm not sure if there are any "edge cases" I am not accounting for.
Is there a better way to do it? The inner function way seems a bit odd, as I don't like how I the promise[method] works.

Here is how the spread method works:

var resolvedArray = Promise.resolve([1, 2, 3]);

resolvedArray.spread(function(a, b, c) {
  console.log(a === 1); // true
  console.log(b === 2); // true
  console.log(c === 3); // true
});

var resolvedOther = Promise.resolve("Hello");

resolvedOther.spread(function(value) {
  console.log(value === "Hello"); // true
});

var rejectedArray = Promise.reject([1, 2, 3]);

rejectedArray.spread(undefined, function(a, b, c) {
  console.log(a === 1); // true
  console.log(b === 2); // true
  console.log(c === 3); // true
});

var rejectedObject = Promise.reject("Hello");

rejectedObject.spread(undefined, function(value) {
  console.log(value === "Hello"); // true
});

And here is my implementation:

Promise.prototype.spread = function() {
  var promise = this;

  var spread = function(promise, method, callback) {
    return promise[method](function(previousValue) {
      if (previousValue instanceof Array) {
        return callback.apply(undefined, previousValue);
      } else {
        return callback.call(undefined, previousValue);
      }
    });
  };

  if (arguments.length >= 1 && arguments[0] !== undefined) {
    promise = spread(promise, 'then', arguments[0]);
  }

  if (arguments.length >= 2 && arguments[1] !== undefined) {
    promise = spread(promise, 'catch', arguments[1]);
  }

  return promise;
};

Your implementation is taking .then(a, b) and mapping it to .then(a).catch(b) which is not exactly the same thing.  An exception or return of a rejected promise in a in your implementation will hit the .catch(b), but in .then(a, b), it will not hit b, so I think your .spread() is not correctly imitating the .then() behavior in some circumstances.

Here's my implementation that should more closely mimic .then(a, b):

Promise.prototype.spread = function(a, b) {

    function doSpread(fn) {
        return function(args) {
            if (typeof fn === "function") {
                if (Array.isArray(args)) {
                    return fn.apply(null, args);
                } else {
                    return fn(args);
                }
            } else {
                return args;
            }
        }
    }
    if (typeof b !== "undefined") {
        // there is a reject handler
        return this.then(doSpread(a), doSpread(b));
    } else {
        // no reject handler
        return this.then(doSpread(a));
    }
}

// Array.isArray polyfill
if (!Array.isArray) {
  Array.isArray = function(arg) {
    return Object.prototype.toString.call(arg) === '[object Array]';
  };
}

Working demo and tests: https://jsfiddle.net/jfriend00/nqc0h10y/

And, here's a version that makes the change to wait on any promises that are returned in the array of values per our discussion in comments. I also changed it so that if you don't pass a function to .spread(), it will reject:

Promise.prototype.spread = function(a, b) {

    function doSpread(fn) {
        return function(args) {
            if (typeof fn === "function") {
                if (Array.isArray(args)) {
                    // in cases where there could be promises in the array, wait on those
                    return Promise.all(args).then(function(returnArgs) {
                        // split the array of results into separately passed arguments
                        return fn.apply(null, returnArgs);
                    });
                } else {
                    return fn(args);
                }
            } else {
                throw new Error("Must pass a function to .spread()");
            }
        }
    }
    if (typeof b !== "undefined") {
        // there is a reject handler
        return this.then(doSpread(a), doSpread(b));
    } else {
        // no reject handler
        return this.then(doSpread(a));
    }
}

// Array.isArray polyfill
if (!Array.isArray) {
  Array.isArray = function(arg) {
    return Object.prototype.toString.call(arg) === '[object Array]';
  };
}

Working demo: https://jsfiddle.net/jfriend00/om5auzs0/


Asynchronous logging using custom log4j appender and ExecutorService I have a requirement to send emails when certain errors are logged(using log4j 1.2.17). But the SMTP appender sends emails in a synchronized manner. I can not use the AsyncAppender either. So I have written my own custom appender and an email sender which utilizes the javax.mail library. The custom appender creates a fixed thread pool of 5 threads using the ExecutorService, and in the custom Appenders append() method I create an object of the email sender class that I have written and submit that to the thread pool.

The Code for the custom appender is as following(some of the non-important segments have been removed eg: some parameters when creating an instance of emailSender)

public class emailAppender extends AppenderSkeleton {

    private static ExecutorService threadPool = null;

    static {
        threadPool = Executors.newFixedThreadPool(5);
    }
    @Override
    protected void append(LoggingEvent event) {
        if(this.closed != true){
            emailSender emailNotification = new emailSender(
                   event.getMessage().toString(),getReceiver());
            threadPool.submit(emailNotification);
        }
    }

    public void close() {
        this.closed=true;
    }

    public boolean requiresLayout() {
        return false;
    }
}

The code for the email sender is as follows

public class emailSender implements Runnable  {

    public emailSender( password, String message, String receiver){
        this.message=message;
        this.receiver=receiver;
    }

    public void run() {
        Properties props = new Properties();
        props.put("mail.smtp.starttls.enable", "true");
        props.put("mail.smtp.port", getSmtpPort());

        Session session = Session.getDefaultInstance(props,
                new javax.mail.Authenticator() {
                    protected PasswordAuthentication getPasswordAuthentication() {
                        return new PasswordAuthentication(getSmtpUsername(),getSmtpPassword());
                    }
                });

        try {
            MimeMessage message = new MimeMessage(session);
            message.setFrom(new InternetAddress(getSmtpUsername()));
            message.addRecipient(Message.RecipientType.TO,new InternetAddress(getReceiver()));
            message.setSubject(getSubject());
            message.setText(getMessage());
            Transport.send(message);
            System.out.println("Notification sent successfully to " + getReceiver() + " for Error : " + getMessage());
        } catch (MessagingException e) {
            e.printStackTrace();
        }
    }

The size of the thread pool was determined considering the maximum number of errors that get logged at one time. And also the downside of the solution being asynchronous is that the logs might not actually get emailed which is acceptable for us.

Currently, this code works fine for me. I would want to know if this code is acceptable for production without considering the fact that the logs might not get emailed due to asynchronous behavior.

The UpperCamelCase format is used to name types. You should rename your classes (and constructors) to EmailAppender and EmailSender.

The ExecutorService has a shutdown method that you can use to reject new tasks and terminate the current ones. You may shutdown your pool form the close method. And there is a nice code sample in the official documentation : https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ExecutorService.html

I guess you should also avoid to use System.out.println and e.printStackTrace(). They will both print unexpected messages on the console. Imagine that your code will be shipped in a library, once a dev will use your library he will suddenly got things printed on his console.

Aside of that I don't see anything wrong.

On a more personal point of view, I would have tried to extract the "configuration" (Creation of Properties and creation of the instance) to another class so that you make a clear distinction between the conversion of the LoggingEvent to an email and the sending of an email message.


Sorting of nested categories inside a Common Table Expression I have a PostgreSQL database which contains (among other things) the following table definition:

CREATE TABLE al_categories
(
  id serial NOT NULL,
  name character varying(400) NOT NULL,
  banner text,
  parent_id integer NOT NULL,
  display_order integer NOT NULL,
  se_name character varying(100) NOT NULL, -- This is what WordPress calls a "slug"
  is_active boolean NOT NULL
  CONSTRAINT al_categories_pkey PRIMARY KEY (id)
)

This creates a category hierarchy stored using adjacency lists.  Top-level categories are stored with parent_id set to 0.

One requirement that we have is to retrieve a list of all of the category IDs, along with the full textual path from the top of the order.  To get this data, I have written a view which executes the following Common Table Expression:

WITH RECURSIVE cats AS (
    SELECT
        id,
        name,
        CAST(name AS varchar(1000)) AS path_name,
        0 AS parent_id,
        1 AS level_indicator,
        display_order::INT8 * 1000000000000 AS global_sort,
        1000000000000 / 1000 AS sort_part_upto,
        is_active
    FROM al_categories
    WHERE parent_id = 0
UNION
    SELECT
        c1.id,
        c1.name,
        CAST(c0.path_name || ' / ' || c1.name AS varchar(1000)) AS path_name,
        c0.id AS parent_id,
        c0.level_indicator + 1 AS level_indicator,
        c0.global_sort + (display_order::INT8 * sort_part_upto) AS global_sort,
        c0.sort_part_upto / 1000 AS sort_part_upto,
        c1.is_active
    FROM al_categories AS c1
        INNER JOIN cats AS c0
        ON c1.parent_id = c0.id
)
SELECT id, name, path_name, parent_id, level_indicator, global_sort
FROM cats
ORDER BY global_sort, path_name

As you can see, one of the requirements is that the data is sorted using the display_order columns from all of the categories, such that each category's global_sort number is also based on the display_order of its parent.

I feel like the global_sort is a big mess, and there has got to be a better way to do it.  Is the way I am doing it a good way, or is there a better way to write this recursive query?

An example dataset (simplified from http://comfortfirst.com/) is here, and an example of the returned data from the view is here.

Instead of using some large number base, I'd use an array.

Start with ARRAY[display_order] AS global_display_order.  For each additional level, array_append(global_display_order, display_order).  Arrays are compared element by element; ties are resolved by putting the shorter array first — which is what you want.

For readability, instead of the table aliases c0 and c1, I suggest parent and child.


The Scripted FizzBuzz Kata I run a coding dojo at work. for one session I'm to showing that you can use a kata to get into a new language.

I'm using the FizzBuzz Kata to show JavaScript (and F#) because it is short.  The problem is my JavaScript knowledge is only slightly above the copy -> paste -> fiddle level.

Thus, I would like to see better ways of doing this (using qunit):

var fizzBuzzer = (function () {
    var For = function (number) {
        var isBuzz = number % 5 === 0;
        var isFizz = number % 3 === 0;

        if ((isFizz) && (isBuzz)) {return "FizzBuzz";}
        if (isBuzz ) {return "Buzz";}
        if (isFizz ) {return "Fizz";}

        return number;
    };

    return { For: For };
})();

test("not a multiple of 5 or 3 returns number", function () {
    equal(fizzBuzzer.For(1), 1);
    equal(fizzBuzzer.For(2), 2);
});

test("multiple of 3 returns Fizz", function () {
    equal(fizzBuzzer.For(3), "Fizz");
    equal(fizzBuzzer.For(6), "Fizz");
});

test("multiple of 5 returns Buzz", function () {
    equal(fizzBuzzer.For(5), "Buzz");
    equal(fizzBuzzer.For(10), "Buzz");
});

test("multiple of 3 and 5 returns FizzBuzz", function () {
    equal(fizzBuzzer.For(15), "FizzBuzz");
    equal(fizzBuzzer.For(30), "FizzBuzz");
});

Structure

I'm not quite sure why you chose to write your program the way you did. It seems to add complexity to the naive approach of using a normal function, without adding any benefits:

function fizzBuzzer(number) {
    var isBuzz = (number % 5) === 0;
    var isFizz = (number % 3) === 0;

    if (isFizz && isBuzz) { return "FizzBuzz"; }
    if (isBuzz) { return "Buzz"; }
    if (isFizz) { return "Fizz"; }

    return number;
}

Misc

parentheses: sometimes you have too many (for example ((isFizz) && (isBuzz)) would be more readable as (isFizz && isBuzz)), and sometimes you could use a bit more (for example, to understand var isBuzz = number % 5 === 0;, you need to remember the precedent of 3 things; I would probably change it to var isBuzz = (number % 5) === 0;).
spaces: I wouldn't put a space before ). But I would put one after { and before } to make the code more readable. So if (isBuzz ) {return "Buzz";} for example would become if (isBuzz) { return "Buzz"; }.
I like that you wrote unit tests for JavaScript code. You could also include a test for 0, and maybe negative numbers (as they are allowed as input to your function).


Fastest way to change the HSV value of all pixels in a bitmap I want to change all the pixels in an image to have the same value, in terms of the HSV colour space.

At the moment I'm running the following code:

private Bitmap changeVForAllPixels(Bitmap bitmap, float newV)
{
    int width = bitmap.getWidth();
    int height = bitmap.getHeight();
    int[] rgbs = new int[width * height];

    bitmap.getPixels(rgbs, 0, width, 0, 0, width, height);

    for(int rgbi = 0; rgbi < rgbs.length; rgbi++)
    {
        float[] hsv = new float[3];
        Color.colorToHSV(rgbs[rgbi], hsv);
        hsv[2] = newV;
        rgbs[rgbi] = Color.HSVToColor(hsv);
    }

    return Bitmap.createBitmap(rgbs, width, height, Bitmap.Config.ARGB_8888);
}

I would like to know if there is a faster way, as this is taking in the range of 30 seconds to process a 1920 x 1080 pixels image on an LG D802 phone. I'd like to get it down to the 1 second range, or to know if that's not possible.

Disclaimer: I am NOT familiar with Java, let alone the Android version. This may not compile or may be slower. I have no way to test this!

First of all, You deserve huge congratulations!!! Even I could read and understand what is going on! From the bottom of my heart: Thank you!

But still, there are a few points that scared me.

One of them is this:

int[] rgbs = new int[width * height];

Why rgbs? Sure, you are storing an RGB representation of a pixel in the form of an integer. So, why not just call it pixels?

Other variable that needs rewording is rgbi. Since it is an incrementer, it should be simply i. No need to complicate.

Based on http://developer.android.com/training/articles/perf-tips.html, you could use the for loop with 'cached' length, to try to speed up your code. I'm not sure if it will work.

It would look like this:

int length = pixels.length;
for(int i = 0; i < length; i++)
{
    float[] hsv = new float[3];
    Color.colorToHSV(pixels[i], hsv);
    hsv[2] = newV;
    pixels[i] = Color.HSVToColor(hsv);
}

You may ask: Why aren't you using for-each?

Since you are setting the value of the pixels again, it won't work. Based on https://stackoverflow.com/questions/15844443/why-java-foreach-doesnt-change-element-value, you couldn't write back the new value since it will create a copy. You could try to get a reference instead (possible in PHP), but I'm not sure if it works in Java.

Besides that, the page also says that for performance-critical loops, this is the best choice.

Your biggest bottleneck is this block:

float[] hsv = new float[3];
Color.colorToHSV(pixels[i], hsv);
hsv[2] = newV;
pixels[i] = Color.HSVToColor(hsv);

This is where everything happens! You could try to use memoization techniques to store already-calculated values. This will speed-up for images with less colors, like a photo of the sun/clean sky or a logo or similars. Photos with areas of colors that are equal or similar.

For images with lots of colors, like a photo of a landscape, it may still produce somewhat good results in RAM. Since there are 16777216 colors that can be generated using RGB values, this would be equivalent to storing 4 * 16777216 = 67108864 Bytes = 65536 KBytes = 64MBytes in your worst case scenario. Since your bitmaps are 1920 x 1080 pixels, you have 1920 * 1080 = 2073600 pixels * 4 = 6220800 bytes = 6075 KBytes ~= 5,9326171875 MBytes of information. So, you don't have to worry about your worst case scenario. As long as the newV doesn't change, all this data is re-usable to other images, being it even faster on the next ones.


Plot random generated 'n' non-colliding circles using Matplotlib I have a function which plots n randomly generated non-colliding circles. The circle is a Circle object, part of matplotlib.patches. We will generate each random circle by its center and r, for example : r = random.uniform(min_r, max_r).

To make circles with no collision, no two circles (with c1, r1 and c2, r2 as the center and radius) can have :
distance(c1, c2) < r1  + r2.

Here is the imports:

import random
import matplotlib.pyplot as plt
from matplotlib.patches import Circle
import numpy

The implementation is done by the function below, how to make the script more compact and the implementation more efficient (faster)? Thanks.

The main function:

def nocollide_random_unif_circles():

    def add_circle(ax, center=(0,0), r=1, face_color=(0,0,1,1), edge_color=(0,0,1,1), line_width = None):
        C = Circle(center, radius = r, facecolor = face_color, edgecolor = edge_color, linewidth = line_width)
        ax.add_patch(C)

    def dist2d(p, q):
        distance = numpy.sqrt( (p[0]-q[0])**2 + (p[1]-q[1])**2 )
        return distance

    def collide(center, rad, centers, rads):
        for c, r in zip(centers, rads):
            if dist2d(c, center) < r + rad:
                return True
        return False

    ncircle = 1000
    min_r = 1
    max_r = 200
    color = 'blue'

    sizex = [0, 1000]; sizey = [0, 1000]

    fig, ax = plt.subplots(1, 1)

    centers = [(random.uniform(sizex[0], sizex[1]), \
               random.uniform(sizey[0], sizey[1]))]
    rads = [random.uniform(min_r, max_r)]
    add_circle(ax, center = centers[0], r = rads[0],\
               face_color = color, edge_color = (0, 0, 0, 0))
    for n in range(ncircle-1):
        center = (random.uniform(sizex[0], sizex[1]), \
               random.uniform(sizey[0], sizey[1]))
        rad = random.uniform(min_r, max_r)
        while collide(center, rad, centers, rads):
            center = (random.uniform(sizex[0], sizex[1]), \
               random.uniform(sizey[0], sizey[1]))
            rad = random.uniform(min_r, max_r)
        centers.append(center); rads.append(rad)
        add_circle(ax, center = centers[-1], r = rads[-1],\
               face_color = color, edge_color = (0, 0, 0, 0))
        print(n)

    ax.axis('equal')
    ax.set_xlim(sizex); ax.set_ylim(sizey)
    ax.tick_params(colors = (0,0,0,0))
    ax.set_title('min radius = {}, max radius = {}, n = {} circles'.format(min_r, max_r, ncircle))

    fig.tight_layout()
    fig.show()

Example result with n=4000 (took quite some time):

I would pull out the generation of the next circle and the generation of all circles into their own functions. This allows you to test those parts without having to plot the result, allowing some speed comparisons:

def place_circle(centers, rads, size_x, size_y, size_r):
    center = random.uniform(*size_x), random.uniform(*size_y)
    rad = random.uniform(*size_r)
    if centers and rads:
        while collide(center, rad, centers, rads):
            center = random.uniform(*size_x), random.uniform(*size_y)
            rad = random.uniform(*size_r)
    return center, rad

def place_circles(ncirle, size_x, size_y, size_r):
    centers, rads = [], []
    for n in range(ncircle):
        center, rad = place_circle(centers, rads, size_x, size_y, size_r)
        centers.append(center)
        rads.append(rad)
        # print(n)
    return centers, rads

You can then use this like this in your main function:

centers, rads = place_circles(ncircle, sizex, sizey, (min_r, max_r))
for center, rad in zip(centers, rads):
    add_circle(ax, center=center, r=rad, face_color=color, edge_color=BLACK)

Note that I made (0, 0, 0, 0) global constant called BLACK and removed the whitespace around = for keywords (as recommended by Python's official style-guide, PEP8).

Now, we can test how long it takes to find a place for 1000 circles:

%timeit place_circles(1000, (0, 1000), (0, 1000), (1, 200))
3.33 s ± 139 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

With this baseline, you can try to improve the performance.


How to improve this fluent interface? I am working on a fluent interface for exception handling.
Here's how it looks :

On<NullReferenceException>()
                    .First.LogTo("errors")
                    .Then.TranslateTo("null object");

            On<FileNotFoundException>()
                    .First.Translate(excpetion => excpetion.Message)
                    .Then.LogTo("errors");

            On<InvalidOperationException>()
                    .If(excpetion => excpetion.Message.Contains("This is a test"))
                    .First.DoSomethingElse(exception => Console.WriteLine(exception.Message));

            On<ArgumentException>().Ignore();

Do you think it's as fluent as it should be ? can I make it better ? any suggestions ?

I would remove the First and Then methods and just execute actions in the order they are declared.


Simple text-to-binary converter written in C++ I am learning C++ on my own and was hoping for some feedback on this simple text-to-binary converter. The program runs and I haven't encountered any errors with it yet. Any input is welcome.

#include <iostream>
#include <string>
#include <bitset>

using namespace std;

int main()
{
char letter = ' ', playAgain = 'y';
string word = " ";

cout << "\t**Text To Binary Convertor**\n\n";

while (playAgain == 'y'){

    cout << "Please enter a character, word, or phrase: ";

    getline (cin, word, '\n');

    cout << "\nThe binary value for " << word << " is \n";

    for (unsigned int wordPosition = 0; wordPosition < word.size(); ++wordPosition){

        letter = word[wordPosition];

        bitset <8> binary(letter);

        cout << binary;
        }

    cout << "\n\nWould you like to try again? (y/n)";

    cin >> playAgain;

    if (playAgain != 'y'){
        cout << "\n\nExiting program.";
        playAgain = 'n';
        }
        cin.ignore();
}

return 0;
}

As already mentioned, your code is looking okay.  Here are my additions:

It's best not to use using namespace std.
Variables should be declared/initialized on their own line:

char letter = ' ';
char playAgain = 'y';

You don't need a newline between every line of code.  This will just make your program appear longer and harder to read.  Just separate lines into groups based on purpose.
Variable declarations/initializations should be placed as close in scope as possible to where they will be first used:

std::cout << "Please enter a character, word, or phrase: ";
std::string word; // declaration is first used here
getline(std::cin, word, '\n');

Your main loop could use for (;;).  This is essentially an infinite loop, but it should do a break at the end if the user inputs a value other than 'Y':

for (;;)
{
    // do stuff

    std::cout << "\n\nWould you like to try again? Y/N)";
    char playAgain;
    playAgain = std::toupper(playAgain);
    std::cin >> playAgain;
    std::cin.ignore();

    if (playAgain != 'Y')
    {
        break;
    }

    // playAgain is 'y', so do stuff again
}

Prefer to use std::string::size_type for the inner for-loop as it's the type returned from std::string::size().

Or if you have C++11, use a range-based for loop instead:

for (auto iter& : word)
{
    letter = iter;

    // ...
}


Custom SQL statement I am just wondering if my code can still be simplified. I intend to make it reusable in all update statements.

Public Sub updateRecord(ByRef procedure As String, ByRef parameters As String, ByRef obj As String, ByRef LastName As String, ByRef FirstName As String, ByRef MiddleInitial As String, ByRef Age As String, ByRef Address As String)
Dim CS As String = ConfigurationManager.ConnectionStrings("DBCS").ConnectionString
Try
    Using con As New SqlConnection(CS)
        Dim cmd As SqlCommand = New SqlCommand(procedure, con)
        cmd.CommandType = System.Data.CommandType.StoredProcedure
        cmd.Parameters.AddWithValue(parameters, obj)
        cmd.Parameters.AddWithValue("@LastName", LastName)
        cmd.Parameters.AddWithValue("@FirstName", FirstName)
        cmd.Parameters.AddWithValue("@MiddleInitial", MiddleInitial)
        cmd.Parameters.AddWithValue("@Age", Age)
        cmd.Parameters.AddWithValue("@Address", Address)
        cmd.Connection = con
        con.Open()
        Dim TotalRows As Int32 = cmd.ExecuteNonQuery()
    End Using
Catch ex As Exception
    MessageBox.Show(ex.ToString, "Annual Procurement Plan", MessageBoxButtons.OK, MessageBoxIcon.Exclamation)
End Try
End Sub

Let us focusing first on the code in question.

Naming

Based on the naming guidelines methods should be named using PascalCase casing and method parameters should be named using camelCase casing.

If you have your own coding/naming guidelines you should stick to them. Right now I don't believe so because you are mixing the casing styles for the input parameter names.

Based on the same guidelines one shouldn't use abbreviations for names. A variable named CS doesn't make it obvious at first glance what it is about, but connectionString would do.

Catching Exception isn't really a good way to do error handling. You should catch only the type of exception which can be handled by your code. What I mean with this can be read here

  Always deal with known exceptions as low-down as you can. However, if you're expecting an exception it's usually better practice to test
  for it first. For instance parse, formatting and arithmetic exceptions
  are nearly always better handled by logic checks first, rather than a
  specific try-catch.
  If you need to do something on an exception (for instance logging or roll back a transaction) then re-throw the exception.
  Always deal with unknown exceptions as high-up as you can - the only code that should consume an exception and not re-throw it should
  be the UI or public API.

If you want to make this reusable for other update methods, you should pass a Dictionary<TKey,TValue> to the method, where as TKey will be the parameters "name" like @LastName and the TValue will be the values the parameter will/should have like so

Public Sub UpdateRecord(ByRef procedure As String, Dictionary<String, Object> parameters)

You can then add the keys as the parameters name and the values as the value.


My first project - mini hangman game I decided to polish my skills a little bit after learning about one dimensional arrays, and built my own mini project - hangman game.
I'd like to have some criticism, I know it's not perfect and far from it, but please be aware, it's my FIRST EVER real project with more than 20 + lines of code.

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

public class Main {
    static Scanner input = new Scanner(System.in);

    public static void main(String[] args) {

        String[] words = {"java", "hello"};
        int random = (int)(0 + Math.random() * words.length);
        String wordToGuess = words[random];
        boolean isGuessed = false;
        int counter=0;
        int maxGuess = 7;
        char[] chars = new char[wordToGuess.length()];
        for (int i =0; i<chars.length; i++)
            chars[i] = '-';
        while (!isGuessed && counter <7) {

            System.out.println("Enter your guess: ");
            char guess = input.next().charAt(0);
            counter++;
            maxGuess--;
            for (int i = 0; i < wordToGuess.length(); i++) {
                if (wordToGuess.charAt(i) == guess)
                    chars[i] = guess;
            }
            if (doesContain(chars)) {
                for (int j = 0; j < wordToGuess.length(); j++) {
                    System.out.print(chars[j] + " ");

                }
                System.out.println("Guess left: " + maxGuess);

            } else {
                System.out.println("Good job! The word is: ");
                for (int j = 0; j < wordToGuess.length(); j++) {
                    System.out.print(chars[j] + " ");
                }
                System.out.print("Number of tries: ");
                System.out.print(counter);
                isGuessed = true;

            }
        }

    }

    public static boolean doesContain(char[] chars) {
        for (int i =0; i<chars.length; i++) {
            if (chars[i] == '-')
                return true;
        }
        return false;
     }

 }

It is necessary to have consistent and correct (as per style convention) white space. This includes vertical alignment (such as indentation) and spacing between and around particular syntax.

Convention on braces vary, but often braces are encouraged for single statement blocks. One reason is that a later revision may not realise to add braces when adding another statement to the block.

The following is how I might format the code. Note that I am not afraid to add extra blank lines to separate structures. I am not opposed to having three blank lines between methods, though I did not do that here.

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

public class Main {

  static Scanner input = new Scanner(System.in);

  public static void main(String[] args) {

    String[] words = {"java", "hello"};
    int random = (int)(0 + Math.random() * words.length);
    String wordToGuess = words[random];
    boolean isGuessed = false;
    int counter = 0;
    int maxGuess = 7;
    char[] chars = new char[wordToGuess.length()];

    for (int i = 0; i < chars.length; i++) {
      chars[i] = '-';
    }

    while (!isGuessed && counter < 7) {

      System.out.println("Enter your guess: ");

      char guess = input.next().charAt(0);

      counter++;
      maxGuess--;

      for (int i = 0; i < wordToGuess.length(); i++) {
        if (wordToGuess.charAt(i) == guess) {
          chars[i] = guess;
        }
      }

      if (doesContain(chars)) {

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(chars[j] + " ");
        }

        System.out.println("Guess left: " + maxGuess);

      } else {

        System.out.println("Good job! The word is: ");

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(chars[j] + " ");
        }

        System.out.print("Number of tries: ");
        System.out.print(counter);

        isGuessed = true;

      }

    }

  }

  public static boolean doesContain(char[] chars) {

    for (int i = 0; i < chars.length; i++) {
      if (chars[i] == '-') {
        return true;
      }
    }

    return false;

  }

}

This blob concerns me.

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;
char[] chars = new char[wordToGuess.length()];

It is best to keep separate what can be kept separate. I took a look at "x depends on y" written as x ⇒ y and came up with this.

chars ⇒ wordToGuess
wordToGuess ⇒ words
wordToGuess ⇒ random
random ⇒ words

Therefore we can break and reorder these variable declarations like so:

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
char[] chars = new char[wordToGuess.length()];

boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;

Then looking down a bit further there is a for-loop which only depends on chars, so we can pull that up.

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
char[] chars = new char[wordToGuess.length()];

for (int i = 0; i < chars.length; i++) {
  chars[i] = '-';
}

boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;

There are seven variables in scope (are accessible) for the rest of main. For each variable one must wonder "does this matter later?". This increases the effort to understand a block of code. Doing a text search I found that words and random are unused later. Therefore, lets more closely contain them.

String wordToGuess;
{
  String[] words = {"java", "hello"};
  int random = (int)(0 + Math.random() * words.length);
  wordToGuess = words[random];
}

char[] chars;
{
  chars = new char[wordToGuess.length()];
  for (int i = 0; i < chars.length; i++) {
    chars[i] = '-';
  }
}

Now the program is well formatted and just by looking at dependencies we have improved the organisation (we did not have to know what the program does). Further improvements can come from rethinking some of how the program does its task. There seems to be quite a bit happening in the while-loop so I want to consider that first.

Reading the code and doing some string searching, I have determined what the while-loop reads and writes.

Reads & Writes

isGuessed
counter
maxGuess
chars
input

Reads Only

wordToGuess

Writes Only

N/A

Lets discover the purpose of each variable which is both read and written. Why these particularly? If a variable is only read it is invariant (unchanging) during the loop. If a variable is only written it has no effect on the loop behaviour.

input is used to read through stdin a character at a time, once each iteration.
counter is increased once per iteration. This is used to print the number of guesses thus far and to terminate the loop if !(counter < 7).
maxGuess is decreased once per iteration. This is used to print the number of guesses remaining.
chars is updated with the guessed letter once per iteration.
isGuessed is set to true only if the guessed word is complete. This is used to terminate the loop if !!isGuessed.

I see a redundancy between counter and maxGuess. counter = 7 - maxGuess or maxGuess = counter + 7. I think counting down to zero from maxGuess makes more sense than counting up to 7, so I will replace counter with maxGuess. Also I rename maxGuess to guessesRemaining and introduce a final variable maxGuesses assigned to 7.

boolean isGuessed = false;
final int maxGuesses = 7;
int guessesRemaining = maxGuesses;

while (!isGuessed && guessesRemaining > 0) {

  System.out.println("Enter your guess: ");

  char guess = input.next().charAt(0);

  guessesRemaining--;

  for (int i = 0; i < wordToGuess.length(); i++) {
    if (wordToGuess.charAt(i) == guess) {
      chars[i] = guess;
    }
  }

  if (doesContain(chars)) {

    for (int j = 0; j < wordToGuess.length(); j++) {
      System.out.print(chars[j] + " ");
    }

    System.out.println("Guess left: " + guessesRemaining);

  } else {

    System.out.println("Good job! The word is: ");

    for (int j = 0; j < wordToGuess.length(); j++) {
      System.out.print(chars[j] + " ");
    }

    System.out.print("Number of tries: ");
    System.out.print(maxGuesses - guessesRemaining);

    isGuessed = true;

  }

}

Final variables cannot be changed, so we know automatically the most the while-loop does with this variable is read it, which it does. Thus we have reduced the number of variables the loop both reads and writes and this can simplify comprehension.

While reading I found the method name doesContain does not suggest its purpose well. Thus I renamed it to isSolved. I also thought chars was too generic of a name so I renamed it to incompleteWord.

I found '-' was used as a placeholder letter in both main and isSolved. We want to avoid magic literals in our program. A "magic literal" is some literal (such as a string, character, or number) which occurs in one or more places and has an non-obvious and unstated reason for being the value it is. When reading code with magic literals we are not sure what the code does. When updating code with magic literals we are not sure where all we need to make the replacement, especially because there may be different reasons for a particular literal. To fix this we can simply name the literal.

static final char placeholderLetter = '-';
...
incompleteWord[i] = placeholderLetter;
...
if (incompleteWord[i] == placeholderLetter) {

There is much from here we can still do. However, I think this is a good place to stop seeing as this is your first substantial program.

I do not think giving advice or answers on how to implement a program from the abstract is on topic. Rather, this Q&A service is for suggesting improvements to code which is already working and understood. If you want to know how to add some ASCII art to your program then I suggest Stack Overflow.

This is my final version of your program after moving a couple more declarations around.

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

public class Main {

  static Scanner input = new Scanner(System.in);

  static final char placeholderLetter = '-';

  static final final int maxGuesses = 7;

  static final String[] words = {"java", "hello"};

  public static void main(String[] args) {

    String wordToGuess;
    {
      int random = (int)(0 + Math.random() * words.length);
      wordToGuess = words[random];
    }

    char[] incompleteWord;
    {
      incompleteWord = new char[wordToGuess.length()];
      for (int i = 0; i < incompleteWord.length; i++) {
        incompleteWord[i] = '-';
      }
    }

    boolean isGuessed = false;
    int guessesRemaining = maxGuesses;

    while (!isGuessed && guessesRemaining > 0) {

      System.out.println("Enter your guess: ");

      char guess = input.next().charAt(0);

      guessesRemaining--;

      for (int i = 0; i < wordToGuess.length(); i++) {
        if (wordToGuess.charAt(i) == guess) {
          incompleteWord[i] = guess;
        }
      }

      if (isSolved(incompleteWord)) {

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(incompleteWord[j] + " ");
        }

        System.out.println("Guess left: " + guessesRemaining);

      } else {

        System.out.println("Good job! The word is: ");

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(incompleteWord[j] + " ");
        }

        System.out.print("Number of tries: ");
        System.out.print(maxGuesses - guessesRemaining);

        isGuessed = true;

      }

    }

  }

  public static boolean isSolved(char[] incompleteWord) {

    for (int i = 0; i < incompleteWord.length; i++) {
      if (incompleteWord[i] == placeholderLetter) {
        return true;
      }
    }

    return false;

  }

}


Print character + the ASCII value, 10 pairs per line My exercise was:

  Write a program that reads input as a stream of characters until encountering
  EOF. Have the program print each input character and its ASCII decimal value.
  Note that characters preceding the space character in the ASCII sequence are
  nonprinting characters. Treat them specially. If the nonprinting character is a
  newline or tab, print \n or \t, respectively.
  Print 10 pairs per line, except start a fresh line each time a newline character is encountered.

This is my code (regarding the special characters, I defined only \n and \t):

#include <stdio.h>
int special_chars(int ch);

int main(void)

{
    int x;
    printf("please enter a some characters, and ctrl + d to quit\n");

    special_chars(x);

    return 0;
}

int special_chars(int ch)

{
    int pairsNum = 0;

    while ((ch = getchar()) != EOF)// testing charecters while not end of file.
    {

    if (ch == '\n')// testing if a control charecter, and printing its
     {
        printf("\\n ");
        pairsNum++;
     }

    else if (ch == '\t')
     {
        printf("\\t ");
        pairsNum++;
     }

    else
    {
        printf("%c,%d ", ch, ch);
        pairsNum++;
    }

    if (pairsNum == 10)// counting the number of outputs, and printing a newline when is 10 (limit).
        printf("\n");
    }
    return ch;
}

A few issues, in no particular order:

put main last to avoid the need for a prototype.
main normally takes argc/argv parameters
special_chars should be static - but it is debateable whether a separate
function is necessary in a short program like this.
main defines variable x and passes it to special_chars.  This is
unnecessary. Just define ch in special_chars and define the function as
taking void (i.e. nothing)
why does special_chars return a value?
indenting is wrong
too many blank lines (8)
missing {} brackets after if (pairsNum == 10)
mixed naming styles (pairsNum should be pairs_num or special_chars should
be specialChars - I prefer the former)
special_chars is a bad name for the function.
pairsNum is an odd name; n_pairs would be my choice.
you don't print a final \n at the end of the program
you don't start a new line after reading a \n, as required - it appears that you do    when you type at the keyboard because entering a \n causes a new line. But if you redirect input into your program it
does not.  Also you don't start a line after each ten, only after the first ten. e.g. if your program executable is called program

$ cat x
abcdefgh
ijklmnop
qrstuvwxyz

$./program < x
please enter a some characters, and ctrl + d to quit
a,97 b,98 c,99 d,100 e,101 f,102 g,103 h,104 \n i,105
j,106 k,107 l,108 m,109 n,110 o,111 p,112 \n q,113 r,114 s,115 t,116 u,117 v,118 w,119 x,120 y,121 z,122 \n $


Image cache for lots of images 2.0 Following from this question

I created a Image cache of sorts (let's call it version 1.0) that used WeakReference as "cache" items. And this comes from the accepted answer:

  It looks like weak reference would serve the purpose of what you are trying to achieve [IF] those images will go out of scope (not being directly referenced in any execution path) after handing them back to the caller of that method then they will be collected by GC at some point.

Here is the code that references on any way the images:

Note: cardBaseId and level are used to identify unambiguously a card, and thereby its image.

On the XAML:

<Image x:Name="Portrait" Style="{StaticResource Portrait}">
    <Image.Source>
        <MultiBinding Converter="{StaticResource PortraitConv}">
            <Binding Path="CardBaseId" />
            <Binding Path="MaxLevel" />
        </MultiBinding>
    </Image.Source>
</Image>

On the IValueConverter:

    public object Convert(object[] values, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        int cardBaseId = (int)values[0];
        int level = (int)values[1];
        return ImageManagerInstance.GetImage(cardBaseId, level);
    }

On ImageManager.cs:

    public BitmapImage GetImage(int cardBaseId, int level)
    {
        Debug.Assert(FilepathManagerInstance != null);

        string filepath = FilepathManagerInstance.GetImageFilepath(cardBaseId, level);
        return new BitmapImage(new Uri(filepath));
    }

So I'd say that the images are not on the scope of my code anymore once they are sent to the XAML.

Still, I've changed the implementation to use MemoryCache as sugested for the reason above, and because this way I have more control of how memory is managed.

Here is the new code:

public class ImageCache
{
    private string Filepath;
    private MemoryCache Cache;
    private TimeSpan SlidingExpiration;

    public ImageCache(string filepathToStoredImages, int cacheMemoryLimitMegabytes, TimeSpan slidingExpiration)
    {
        Filepath = filepathToStoredImages;
        NameValueCollection config = new NameValueCollection();
        config.Add("CacheMemoryLimitMegabytes", cacheMemoryLimitMegabytes.ToString());
        Cache = new MemoryCache("ImageCache", config);
        SlidingExpiration = slidingExpiration;
    }

    public BitmapImage GetImage(int id)
    {
        string identifier = id.ToString();
        BitmapImage image = Cache.Get(identifier) as BitmapImage;

        if (image == null)
        {
            image = GetFromDisk(id);
            CacheItemPolicy policy = new CacheItemPolicy();
            policy.SlidingExpiration = SlidingExpiration;
            Cache.Set(identifier, image, policy);
        }

        return image;
    }

    private BitmapImage GetFromDisk(int id)
    {
        Uri uri = new Uri(Filepath + id + ".png");
        return new BitmapImage(uri);
    }
}

Is the first time I use the MemoryCache class so any insight if this code is correct or something should be changed or improved?

Just a small thing: consider using object and collection initializers.

So this:

NameValueCollection config = new NameValueCollection();
config.Add("CacheMemoryLimitMegabytes", cacheMemoryLimitMegabytes.ToString());

Becomes (also using var):

var config = new NameValueCollection { { "CacheMemoryLimitMegabytes", cacheMemoryLimitMegabytes.ToString() } };

And this:

CacheItemPolicy policy = new CacheItemPolicy();
policy.SlidingExpiration = SlidingExpiration;

Becomes:

var policy = new CacheItemPolicy { SlidingExpiration = SlidingExpiration };

And if you do that, you might want to get rid of the variables, and just specify the value of the parameter directly.

Also, it's common to use camelCase, not PascalCase, for private fields.


fold expressions using a function recursively I have written an accumulate function similar to std::accumulate that works on a std::array. The goal is to remove the loop using fold expressions.
This code currently works in c++17:
#include <array>

template<class U, class F>
class array_accumulate_helper
{
public:
    array_accumulate_helper(U& u, F f)
        : r(u), f(f)
    {
    }

    array_accumulate_helper& operator<<(const U& u)
    {
        r = f(r, u);
        return *this;
    }

private:
    F f;
    U& r;
};

template<class U, std::size_t N, class F, std::size_t... Is>
constexpr U array_accumulate_impl(const std::array<U, N>& u, U r, F f, std::index_sequence<Is...>)
{
    array_accumulate_helper h(r, f);
    (h << ... << u[Is]);
    return r;
}

template<class U, std::size_t N, class F>
constexpr U accumulate(const std::array<U, N>& u, U r, F f)
{
    return array_accumulate_impl(u, r, f, std::make_index_sequence<N>());
}

template<class U, std::size_t N>
constexpr U accumulate(const std::array<U, N>& u)
{
    return accumulate(u, {}, [](const U& a, const U& b) { return a + b; });
}

Is it possible to rewrite array_accumulate_impl using a fold expression that doesn't use array_accumulate_helper?
I can use a fold expression with an operator.
ie. using << in array_accumulate_impl above.
I have been unable to write one using a function recursively.
ie. for a std::array<int, 3> it should expand to:
f(f(f(r, a[0]), a[1]), a[2]);

Unfortunately c++ fold expression supports only binary operators: "any of the following 32 binary operators: + - * / % ^ & | = < > << >> += -= = /= %= ^= &= |= <<= >>= == != <= >= && || , . ->*." So you can't call your custom function in pack expansion without using a wrapper.
But if your function accepts a variadic number of arguments you can use std::apply
static constexpr std::array arr{ 1, 2, 3, 4, 5, 6, 7};

static constexpr auto sum = [](auto&&... items) {
  return (... + items);
};

static constexpr auto accumulate = [](auto&& fn, const auto& u) {
  return std::apply([fn = std::forward<decltype(fn)>(fn)](auto... item) { return fn(item...); }, u);
};

constexpr auto result = accumulate(sum, arr);

Example here.
Otherwise, you can take advantage of the comma operator:
static constexpr std::array arr{ 1, 2, 3, 4, 5, 6, 7};

static constexpr auto accumulate_impl = [](const auto& a, auto init, auto&& fn) {
  return std::apply([fn = std::forward<decltype(fn)>(fn), &init](auto... items) {
    ((void)(init = fn(std::move(init), items)), ...);
    return init;
  }, a);
};

template<class U, std::size_t N>
static constexpr U accumulate(const std::array<U, N>& u) {
  return accumulate_impl(u, U{},  [](auto&& a, auto&& b){return a+b;});
}

static constexpr auto result = accumulate(arr);

Example here.


Python implementation of Kahn's Algorithm Please provide any suggestions on the below code for Kahn's algorithm. Can it be implemented in a better manner, or be improved:

def kahn(graph):
    in_degree = {u : 0 for u in graph}
    for vertices, neighbors in graph.items():
        in_degree.setdefault(vertices, 0)
        for neighbor in neighbors:
            in_degree[neighbor] = in_degree.get(neighbor, 0) + 1

    no_indegree_vertices = {vertex for vertex, count in in_degree.items() if count == 0}

    topological_sort = []
    while no_indegree_vertices:
        vertex = no_indegree_vertices.pop()
        topological_sort.append(vertex)
        for neighbor in graph.get(vertex, []):
            in_degree[neighbor] -= 1
            if in_degree[neighbor] == 0:
                no_indegree_vertices.add(neighbor)

    if len(topological_sort) != len(in_degree):
        print("Graph has cycles; It is not a directed acyclic graph ... ")
        return None
    else:
        return topological_sort

Test Data:

test_graph1 = {
    'A' : set(['B','S']),
    'B' : set(['A']),
    'C' : set(['D','E','F','S']),
    'D' : set(['C']),
    'E' : set(['C','H']),
    'F' : set(['C','G']),
    'G' : set(['F','S']),
    'H' : set(['E','G']),
    'S' : set(['A','C','G'])
}

test_graph2 = {
    'A': [],
    'B': ['A'],
    'C': ['B']
}

test_graph3 = {
    5: [2],
    5: [0],
    4: [0],
    4: [1],
    2: [3],
    3: [1]
}

test_graph4 = {
    1: [2],
    2: [3],
    4: [2],
    5: [3]
}

Be explicit about the data structure you're using, and the assumptions. For example, a comment like this would be helpful for any future readers(including yourself): "We represent a graph as adjacency list stored in a Python dictionary. The adjacency list can contain nodes that are not present in the keys of the dictionary, and they should be treated as nodes that contain no outward edges."
I suspect that it's a bad idea to allow nodes that are not present in the keys. Conventionally, an adjacency list should have one entry for each node, whether they have outward edges or not. Moreover, it causes a lot of headache in implementations(To access a dictionary, we had to use get() method instead of the more straightforward square bracket notation).
The first five lines of your function essentially counts each nodes' occurrence. If you require that all nodes should be present in the keys of the dictionary, it can be implemented in a single line:

in_degree = {u: sum(u in v for v in graph.values()) for u in graph}

topological_sort sounds like a verb. Try use a noun for variable names, such as sorted_nodes.


Optimising a LinkedList data structure - Part 1 I have implemented a linked list.  I feel I overdid it with pointers, used old standards and C++11, and I ended with too many lines of code. I will make my quotations into two parts: one for the Node and iterator struct and the other for rest of the class.

For node and iterator, how can I optimize for C++11?  Can you review it?

#include <vector>
#include <cassert>
template <typename T_>
class LinkedList
{
private:
    struct Node
    {
        T_ data;
        Node* prev;
        Node* next;
        /*Node(T_ D, Node* p = nullptr, Node* n = nullptr) : data(d), prev(p), next(n) {}*/

        Node( const T_ & d = Object{ }, Node * p = nullptr, Node * n = nullptr )
          : data(d), prev(p), next(n) { }

        Node( T_ && d, Node * p = nullptr, Node * n = nullptr )
          : data( std::move(d)), prev(p), next(n) { }

        Node( Node && n ) {        // Rvalue move ctor
            data = std::move(n.data);
            prev = std::move(n.prev);
            next = std::move(n.next);
        }
        Node& operator = ( Node && n ) { // Rvalue = operator
            data = std::move(n.data);
            prev = std::move(n.prev);
            next = std::move(n.next);
            return *this;
        }
        void swap(Node& rhs) throw ()
        {
            std::swap(prev, rhs.prev); std::swap(next, rhs.next);
        }
    };

public:
    struct LinkedListIterator : public std::iterator<std::forward_iterator_tag, T_, int>
    {
        Node*  _pNode;

        LinkedListIterator(Node* p = nullptr) : _pNode(p) {}

        Node* getNode() { return _pNode; }

        T_ operator * () { return _pNode->data; }
        LinkedListIterator & operator ++ () { _pNode = _pNode->next; return *this; } // pre
        LinkedListIterator operator ++ (int) { LinkedListIterator retval = *this; ++*this; return retval; } // post
        bool operator < ( LinkedListIterator const& rhs ) const { return _pNode < rhs._pNode; }
        bool operator != ( LinkedListIterator const& rhs ) const { return _pNode != rhs._pNode; }
        bool operator == ( LinkedListIterator const& rhs ) const { return _pNode == rhs._pNode; }
    };

There are several small things I can see in class Node:

You have a move constructor, but no copy constructor.
You have a move assignment operator, but no copy assignment operator.
Since you use C++11, you should replace the exception specification throw () (now deprecated) by noexcept.
Your move assignment operator is not protected against self-assignment. You should add the condition if (&n != this) { ... }.
Also, if possible, your move constructor and operator= should be noexcept too.
Generally speaking, with move assignment operator, you don't want to move pointers, you only want to swap them since - I believe - your nodes own the pointed values (more information here).

Node& operator=(Node&& n) noexcept {
    if (&n != this) {
        std::swap(data, n.data);
        std::swap(prev, n.prev);
        std::swap(next, n.next);
    }
    return *this;
}


Calculating a factorial with parallel sub-computations using pmap, pvalues and pcalls In an attempt to learn more about Clojure's parallel computation functions, I devised the following task: calculate a factorial in parallel using pmap, pcalls and pvalues.

The idea is to split the factorial sequence into sub-computations, reducing the final output of each.

Here is the obvious non-parallel function:

(defn factorial [n] (reduce * (range 1 (inc n))

pmap is fairly straight-forward:

(defn pmap-factorial [n psz]
  (let [parts (partition-all psz (range 1 (inc n)))]
    (reduce * (pmap #(apply * %) parts))))

However, with pvalues I had to resort to macros that return a computation function.

(defmacro pvalues-factorial [n psz]
  (let [exprs (for [p (partition-all psz (range 1 (inc n)))]
                 (cons '* p))]
    `(fn [] (reduce * (pvalues ~@exprs)))))

Similar approach with pcalls:

(defmacro pcalls-factorial [n psz]
  (let [exprs (for [p (partition-all psz (range 1 (inc n)))]
                     `(fn [] (* ~@p)))]
    `(fn [] (reduce * (pcalls ~@exprs)))))

So, I had to use macros for pcalls and pvalues. Does not seem ideal, but it was the only way I could accomplish the above (and perhaps indicates that pmaps is the best practice for this use case).

Questions:

1) How could the code be made more correct / idiomatic?

2) Did I have to resort to macros for pvalues and pcalls, or have I overlooked another way of accomplishing my goal with these functions?

1) that is quite idomatic Clojure from what I have seen.

2) macros are "contagious" if the thing you are trying to call is a macro and you want to do anything requiring first-class-functions (map apply reduce etc...) then the thing doing the applying must it's self be a macro.

This is why the coding standards for clojure.contrib stable releases require that your library provide a non-macro way to access it's services in addition to the macro one. as your problem nicely demonstrates.
macros are incredibly useful, and they are not first class


Cleaning Netflix-style recommendation engine datasets I am working on a portfolio project that deals with creating a Netflix-style recommendation engine for movies. This code is currently ran locally but I will need to upload it into Jupyter Notebooks and implement it with Amazon SageMaker. The .tsv files are imported from IMDb Datasets.
The code below is working code created by myself. Is there any room for improvement from an efficiency/cleanliness aspect?
import pandas as pd
import numpy as np

basics = pd.read_table('basics.tsv')
origin = pd.read_table('akas.tsv')
ratings = pd.read_table('ratings.tsv')

#setting 'basics' columns
basics.drop(basics.columns[[3,4,6,7]], axis=1, inplace=True)
basics.columns = ['tconst','type','title','year','genre']

#removing all non-movies from index
cond1 = basics['type'].isin(['movie','tvMovie'])

#setting new 'basics' variable
basics2 = basics[cond1]

#setting 'origin' columns
origin.drop(origin.columns[[1,2,5,6,7]], axis = 1, inplace = True)
origin.columns = ['tconst','region','language']

#removing non-english movies from index
cond2 = origin['region'].isin(['US','GB'])
cond3 = origin['language'] == 'en'

#setting new 'origin' variable
origin2 = origin[cond2 | cond3]

#setting 'ratings' columns
ratings.columns = ['tconst','rating','votecount']

#converting strings to integers
ratings.rating = pd.to_numeric(ratings.rating, errors='coerce').astype(np.int64)
ratings.votecount = pd.to_numeric(ratings.votecount, errors='coerce').astype(np.int64)

#setting new 'ratings' variable
ratings2 = ratings[(ratings['rating'] >= 5) & (ratings['votecount'] >= 50)]

#finalizing movie recommendation list
rcmd = basics2.merge(origin2,on='tconst').merge(ratings2,on='tconst')
rcmd.drop_duplicates(subset = 'tconst', keep ="first", inplace = True)
print(rcmd)

Thanks in advance.

The usual
Use a PEP8 linter; move global code into functions; use type hints; use a __main__ guard.
Column use
basics.drop(basics.columns[[3,4,6,7]], axis=1, inplace=True)
basics.columns = ['tconst','type','title','year','genre']

really shouldn't be necessary. Use the usecols kwarg of read_table instead; the col names are a loose guess from the IMDB documentation:
basics = pd.read_table(
    'basics.tsv',
    usecols=(
        'tconst', 'titleType', 'primaryTitle', 'startYear', 'genres',
    ),
)


Collecting Parameter vs overloading addition I'm reading TDD By Example by Kent Beck (which I highly recommend, if you haven't read it!) In The xUnit Example, he discusses how to collect the results of multiple tests in a single TestResult. His code looks like this:

# in TestCase
def run(self, result):
    result.testStarted()
    self.setUp()
    try:
        method = getattr(self, self.name)
        method()
    except:
        result.testFailed()

# in TestSuite
def run(self, result):
    for test in self.tests:
        test.run(result)

In short, Beck passes in the currently running TestResult and asks the tests to add their results to it; the TestSuite acts in much the same way. (Beck says the TestSuite is a Composite, and the pattern of passing in the TestResult is called Collecting Parameter.) In other words, somewhere at the top of the call stack, something is creating a single TestResult which gets passed all the way down to the TestCase.

I find the single TestResult object aesthetically distasteful - I generally don't like sharing state between different components. It smacks of C#'s out keyword (which is not a compliment).

My approach would be to return a TestResult from every TestCase, and allow TestResults to be added to one another:

# in TestResult
def __init__(self, runCount=0, errorCount=0):
    self.runCount = runCount
    self.errorCount = errorCount
def __add__(self, other):
    runCount = self.runCount + other.runCount
    errorCount = self.errorCount + other.errorCount
    return TestResult(runCount, errorCount)

# in TestCase
def run(self):
    result = TestResult()
    result.testStarted()
    self.setUp()
    try:
        method = getattr(self, self.name)
        method()
    except:
        result.testFailed()
    return result

# in TestSuite
def run(self):
    result = TestResult()
    for test in self.tests:
        result += test.run()
    return result

How would you weigh up these two approaches? The state of any given instance of TestResult is now local to the methods. On the other hand, there's more duplication in mine - every method now creates and returns a new TestResult, which might make writing a new implementation of run harder work.

One benefit of a collecting parameter is writers can add multiple items to the list.  The collecting parameter can descend deeper into the stack and each method is free to add as many items to the list as necessary.

If what was added to the list was restricted to the return value of the method, then it becomes the sender's responsibility to know how to add the receiver's return values to the list, and the receiver cannot return another value other than what would have been added to it.

The collecting parameter seems more flexible because any number of receivers may write to it and still be able to return values.


Idiomatic and Conventional F# As of late I have been learning F#. Today I accumulated what I have learned so far to develop my first program - a script that counts the number of lines of code in a given Visual Studio project.

The code for said program is as follows:

#r "System.Xml.Linq"

open System
open System.IO
open System.Xml.Linq
open System.Text.RegularExpressions

printf "Enter solution path: "
let solutionPath = Console.ReadLine()
let solutionDirPath = Path.GetDirectoryName(solutionPath)

let solutionText = File.ReadAllText(solutionPath)
let projectPaths  =
    Regex.Matches(solutionText, "([^\"]*csproj)")
    |> Seq.cast<Match>
    |> Seq.map (fun m -> Path.Combine(solutionDirPath, m.Groups.[1].Value))

let codeFilePaths =
    projectPaths
    |> Seq.map (fun projPath ->
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        document.Descendants()
            |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))
        |> Seq.concat

let locIsSignificant (loc : string) =
    let sanitizedLoc = loc.Trim()
    match sanitizedLoc with
    | ""  -> false
    | "{" -> false
    | "}" -> false
    | _   -> true

let totalLoc =
    codeFilePaths
    |> Seq.sumBy (fun path ->
        File.ReadLines(path)
        |> Seq.where locIsSignificant
        |> Seq.length)

printfn "Total Loc: %i" totalLoc
Console.ReadKey() |> ignore

I have tried write both clean and idiomatic code but I still have some concerns, namely:

In this script, I introduce sequential intermediate bindings (projectPaths->codeFilePaths->totalLoc) however, the binding locIsSignificant feels like an interruption to this sequence. Have I arranged my code in an conventional way?
Is my code as idomatic as can be? That is, could I make the code more expressive by using constructs or commands that I have not considered?

Of course, any additional feedback is welcome/encouraged.

Regex.Matches(solutionText, "([^\"]*csproj)")
|> Seq.cast<Match>
|> Seq.map (fun m -> Path.Combine(solutionDirPath, m.Groups.[1].Value))

Why are you using groups? Since the whole match is a single group, you can remove the parentheses from the expression and then directly use m.Value.

Also, you should probably check that the name ends in .csproj, not just csproj and that it's actually the end of the file name.

let codeFilePaths =
    projectPaths
    |> Seq.map (fun projPath ->
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        document.Descendants()
            |> Seq.where (fun desc -> desc.Name.LocalName = "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.FirstAttribute.Value)))
        |> Seq.concat

I think the complicated map lambda makes this hard to read, I would instead write it as a local named function. If you don't want to do that, at least make sure to indent your code properly (the first |> Seq.map and the |> Seq.concat should be on the same level).
Instead of the where, you can use the overload of Descendants that accepts a node name (but this means you have to use XML namespaces).
Instead of relying on the first attribute being the right one, I would access the attribute by name.

With all these changes and John Palmer's suggestion about Seq.collect, the code becomes:

let codeFilePaths =
    let codeFilesForProject projPath =
        let projectDirPath = Path.GetDirectoryName(projPath)
        let document = XDocument.Load(projPath)
        let ns = XNamespace.Get "http://schemas.microsoft.com/developer/msbuild/2003"
        document.Descendants(ns + "Compile")
            |> Seq.map (fun desc -> Path.Combine(projectDirPath, desc.Attribute(XName.Get "Include").Value))
    projectPaths |> Seq.collect codeFilesForProject


Database Connection and Query methods I have my class DBFuntcions containing this 2 methods:

The method for the connection to the database:

Connection connection_db(String username, String password, String dbname, Connection c) {
    try {
        Class.forName("org.postgresql.Driver");
        c = DriverManager.getConnection("jdbc:postgresql://localhost:5432/"
                + dbname, username, password);
        System.out.println("Opened database successfully");
    } catch (ClassNotFoundException | SQLException e) {
        System.err.println(e.getClass().getName() + ": " + e.getMessage() + "\n\n\n");
    }
    return c;
}

And the method for the query.

void viewAll(String tableName, long startDate, long endDate, Connection c, String path) {
    String query = "SELECT * FROM " + tableName + " WHERE CAST(receivedtime AS integer) BETWEEN ? AND ? ORDER BY source, receivedtime;";

    try (PreparedStatement stmt = c.prepareStatement(query)) {              //try with resources
        stmt.setLong(1, startDate);                                         //prepare query
        stmt.setLong(2, endDate);
        ResultSet rs = stmt.executeQuery();

        try {
            while (rs.next()) { ... }

My question it is the best to open and close the connection?

Just before calling the query and closing right after?
Opening and closing the connection inside the query method?
Or it is better to open it in the main and close it at the end?

I think it should depends of the number of time I try to access to it. Let's say for the moment I only have this query and run it only once.

Inconsistent method naming

Connection connection_db(String username, String password, String dbname, Connection c) {
void viewAll(String tableName, long startDate, long endDate, Connection c, String path) {

These methods have both different naming schemes, the top one uses all_lowercase_with_underscores, while the button user pascalCase. The latter is recommend in Java.

URL Injection using dbname

 c = DriverManager.getConnection("jdbc:postgresql://localhost:5432/"
            + dbname, username, password);

You never check for special characters in the dbname. This can cause databases containing special characters to mess up your function, especially when the database can be edited though config. Escape it using URLEncoder.encode


Calculating Parking Fees Here's the challenge description:

  A parking garage charges a $2.00 minimum fee to park for up to three hours.
  The garage charges an additional $0.50 per hour for each hour or part thereof in excess of three hours.
  The maximum charge for any given 24 hour period is $10.00.
  Assume that no car parks for longer than 24 hours at a time.

  Write a Java program that calculates and displays the parking charges for each customer who parked in the garage yesterday.
  Input the number of hours parked for each customer. The number of customers can be fixed using a final constant.

  The program should display the charge for the current customer and should calculate and display the running total of the day's receipts.
  Use a method called calculateCharges to determine the charge for each customer.

Here's my code:

import java.text.DecimalFormat;
import java.util.Scanner;

public class Hwk4B {

    public static void main(String[] args)
    {

        Scanner keyboard = new Scanner(System.in);
        DecimalFormat formatter = new DecimalFormat("$##.00");

        System.out.print("Enter the number of cars parked yesterday: ");
        final int NUMBER_OF_CARS = keyboard.nextInt();

        double hoursParked=0;
        double currentCost=0;
        double totalCost=0;

        for(int count = 1; count<=NUMBER_OF_CARS; count++)
        {
            System.out.print("Enter hours parked for car " + count + ": ");
            hoursParked = keyboard.nextDouble();
            currentCost = calculateCharges(hoursParked); //most recent customer
            totalCost+=currentCost; //Running total
        }

        System.out.println("Most recent customer paid: "   +  formatter.format(currentCost));
        System.out.println("Running total of receipts: " + formatter.format(totalCost));

    }

    public static double calculateCharges (double numHours)
    {

        double garageCost = 0;

        if(numHours<=3)
            garageCost = 2;

        else if(numHours>3 && numHours<=19)
            garageCost = 2.0 + 0.5*(numHours - 3);

        else if (numHours >19)
            garageCost = 10;

        return garageCost;
    }

}

Your code should be as similar to the spec as possible so:

else if (numHours >19)
     garageCost = 10;

Should be removed and you should add:

return Math.min(garageCost, 10)

My final version is:

public static double calculateCharges (double numHours)
{
    if (numHours <= 3)
    {
         return 2;
    }
    double garageCost = 2.0 + 0.5 * (numHours - 3);
    return Math.min(garageCost, 10);
}