Printing facility for std::chrono::time_point Motivation:

It is known that std::time_point<...> cannot be streamed directly using operator<<(), which is pretty disappointing. Although Howard Hinnant created a good library, I'd like to have sole ownership of all of the code in my library, so I could do some benchmarks without including extra license and setup repositories faster.

Description:

Mainly it is only a streaming operator, which actually uses some C date-time library under the hood. The approach creates a huge chance for data race if std::chrono::time_point<...>s are streamed from multiple threads, even into multiple streams, because std::localtime() can return a static variable.

The formatting string is shared as well, which increases burden on users.

The streaming operator is templated only on the duration type, since casting between clocks which produced time point is not possible via std::chrono::time_point_cast<>().

Code:

#include <iosfwd>
#include <chrono>
#include <iomanip>
#include <ctime>
#include <string>

namespace shino
{
    namespace detail
    {
        std::string time_format{"%Y %b %d %H:%M:%S"};
    }

    std::string time_format()
    {
        return detail::time_format;
    }

    void set_time_format(const std::string& format)
    {
        detail::time_format = format;
    }
}

template <typename Duration>
std::ostream& operator<<(std::ostream& os,
                         const std::chrono::time_point<std::chrono::system_clock, Duration>& timep)
{
    using system_clock_duration = std::chrono::system_clock::duration;
    auto converted_timep = std::chrono::time_point_cast<system_clock_duration>(timep);
    auto seconds_since_epoch = std::chrono::system_clock::to_time_t(timep);
    os << std::put_time(std::localtime(&seconds_since_epoch), shino::time_format().c_str());
    return os;
}

Small demonstration:

#include <iostream>

int main()
{
    std::cout << "current date and time: "
              << std::chrono::system_clock::now() << '\n';

    shino::set_time_format(shino::time_format() + " %a"); //add day of the week
    std::cout << "current date and time with day of the week: "
              << std::chrono::system_clock::now() << '\n';

    return 0;
}

Result:

current date and time: 2017 Jun 17 16:01:27
current date and time with day of the week: 2017 Jun 17 16:01:27 Sat

Global singleton format looks a bit peculiar. Unfortunatelly, iostreams do not support user-defined stream properties like time format, but you can do something similar:

struct format
{
    std::string format;
    time_point time;
};

ostream & operator << (ostream & os, format const & f)
{
    // output f.time using f.format
    // ...
}

Which can be used like

std::cout << format{"%Y %b %d %H:%M:%S %a", now()} << std::endl;

Alongside with operator<<(ostream &, time_point const &) (which just uses some default predefined format) this can be used in a lot cleaner way.

(obviously, all this should be put to appropriate namespaces)


Getting location using LiveData and FusedLocationProviderClient My goal is to get the location when the app starts, and if the user navigates away and comes back, to receive one location update and that's it. I am worried about removing the observer in the getLocationUpdates method.

BoundLocationManager.class

public class BoundLocationManager extends LiveData<Location> {

    private static BoundLocationManager instance;
    private FusedLocationProviderClient mFusedLocationClient;
    private LocationRequest mLocationRequest;

    public static BoundLocationManager getInstance(Context appContext) {
        if (instance == null) {
            instance = new BoundLocationManager(appContext);
        }
        return instance;
    }

    private BoundLocationManager(final Context appContext) {
        mFusedLocationClient = LocationServices.getFusedLocationProviderClient(appContext);
        createLocationRequest();
        mFusedLocationClient.getLastLocation().addOnCompleteListener(new OnCompleteListener<Location>() {
            @Override
            public void onComplete(@NonNull Task<Location> task) {
                if(task.isSuccessful() && task.getResult() != null) {
                    setValue(task.getResult());
                } else {
                    mFusedLocationClient.requestLocationUpdates(mLocationRequest, mLocationCallback, null);
                }
            }
        });

    }

    private void createLocationRequest() {
        mLocationRequest = new LocationRequest();
        mLocationRequest.setInterval(10000);
        mLocationRequest.setFastestInterval(5000);
        mLocationRequest.setPriority(LocationRequest.PRIORITY_HIGH_ACCURACY);
    }

    LocationCallback mLocationCallback = new LocationCallback() {
        @Override
        public void onLocationResult(LocationResult locationResult) {
            for (Location location : locationResult.getLocations()) {
                if (location != null)
                    setValue(location);
            }
        }
    };

    @Override
    protected void onInactive() {
        super.onInactive();
        if (mLocationCallback != null)
            mFusedLocationClient.removeLocationUpdates(mLocationCallback);
    }

}

WeatherFragment.class

private void getLocationUpdates() {
        BoundLocationManager.getInstance(getActivity().getApplicationContext()).observe(this, new Observer<Location>() {
            @Override
            public void onChanged(@Nullable Location location) {
                if (location != null) {
                    getWeather(location.getLatitude(), location.getLongitude());
                    getAddress(location.getLatitude(), location.getLongitude());
                    BoundLocationManager.getInstance(getActivity().getApplicationContext()).removeObserver(this);
                } else
                    Toast.makeText(getActivity(), "Location is null", Toast.LENGTH_SHORT).show();
            }
        });
    }

And finally I use it:

@Override
    public void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        if(ContextCompat.checkSelfPermission(getActivity(), Manifest.permission.ACCESS_FINE_LOCATION) != PackageManager.PERMISSION_GRANTED)
            requestPermissions(new String[]{Manifest.permission.ACCESS_FINE_LOCATION}, MY_PERMISSIONS_REQUEST_LOCATION);
        else {
            getLocationUpdates();
        }
    }

Let me explain how to work with LiveData - ViewModel and Activity.

Right now your BoundLocationManager looks good. I'd recommend not to make it's singleton. And also not consuming it directly in your UI (in this case WeatherFragment) code.

It's highly recommended to keep LiveData in ViewModel and your ViewModel will provide it the UI.

Also if you use LiveData, then never worry about removing the observer. It's done automatically for you.

Let me know if you still have some queries.


Find the kth element which is either divisible by l or r or both This was asked to me in one of interview, so can someone please have a look is my code efficient or we can further improve it.

Example

Input  l=2 r=3 k=10

Output:  15

Explanation 15 is at the 10 position of number divisible by 2 or 3 or both.

{2, 3, 4, 6, 8, 9, 10, 12, 14, 15} 15 is at 10th position.

Assumption: L < R

Code

int main()
{
    long long int l, r, k;
    cin>>l>>r>>k;
    int lValue=l*k;
    int numberofElemtsin_l_Range=k+lValue/r - lValue/(l*r);
    if(numberofElemtsin_l_Range>=k)
    {
        int i, j;
        for(i=numberofElemtsin_l_Range, j=lValue; i>=k; j--)
        {
            if(j%l==0 || j%r==0) i--;
        }
        cout<<j+1;
    }
    else
    {
        int rValue = rValue*k;
        int numberofElemtsin_R_Range= k + rValue/l - rValue/(l*r);
        if(numberofElemtsin_l_Range>=k)
        {
            int i, j;
            for(i=numberofElemtsin_R_Range, j=rValue; i>=k; j--)
            {
               if(j%l==0 || j%r==0) i--;
            }
            cout<<j+1;
        }
    }
    return 0;
}

First, there's a couple of things you're not doing which I think you should: you should provide a comment describing what this program does, and you should validate your inputs.

That is, give a banner like:

/* Given inputs L, R, and K, print the Kth element of the sequence
   of integers made up of all positive multiples of L and/or R, in
   ascending order. */

And validate the inputs with an assert, an error message, or just by swapping L and R if they're not in order. Also, ensure the numbers are positive.

Another approach

With that said, I'd like to encourage you to think about the problem a different way: assuming that L < R, what happens when you add a multiple of R in the sequence?

Let's use 5 and 11 instead of 2 and 3, because they have a bunch of problems and they produce numbers that are all fairly distinct when you divide them. (They're a better example than 2 and 3, or 3 and 7 which were the first two examples I tried. ;-)

If you have L=5, R=11, K=10, consider adding a multiple of R to the sequence:

sequence = {}                // empty sequence
add a multiple of R          // R is 11, remember
sequence = {5, 10, 11}

We'll call that a "block". Adding a "block" adds all the numbers up to the next multiple of R.

Each multiple of 11 brings with it 11/5 = 2+1/5 multiples of 5, because 11/5 == 2+1/5. But when the fractions finally add up to a whole number, it won't count! That's when the multiple of 11 will also be a multiple of 5 (55, 110, 165, etc.) and the numbers aren't added to the sequence more than once. So really, adding a multiple of 11 to the sequence adds 1 (the multiple of 11) plus floor(11/5) == 2 multiples of 5 to the sequence.

In general, adding a "block" -- that is, adding one multiple of R -- to the sequence adds 1 + floor(R/L) numbers to the sequence. We'll call that the "block size" or blocksize.

blocksize = 1 + int(R/L);

We can use this in reverse: If K=10, as above, then K/blocksize == K/3 == 3 (remainder 1). So the 10th value in the sequence would be 3 blocks plus one non-block number. That is:

sequence = {
    /*block 1*/ 5, 10,11,
    /*block 2*/ 15,20,22,
    /*block 3*/ 25,30,33,
    /*block 4*/ 35,         // <-- K=10th number
                   40,44,
    /*block 5*/ 45,50,55,
    /*block 6*/ 60,65,66,
}

The only tricky part will be finding the fractional-block numbers, since you can't just add 5 to 33.

This leaves us with two cases: the cases where K is a multiple of the block size, and cases where there is a remainder.

If K is a multiple of the block size, then the result is simply

R * (K/blocksize)

For the 9th number in the sequence, we'd have K/blocksize == 3, and R * 3 == 33, as shown above.

If K is not a multiple of the block size, we have to find a multiple of L that is less than or equal to the highest number in the previous block, then add L * remainder(K/blocksize) to it.

So if K=10, as above, we look for the multiple of L (5) that is less than or equal to the highest number in block 3. 3 * R == 33, so we'd use 30, then add remainder(10/3) == 1 multiple of 5: 30 + 1*5 = 35.

Similarly, if K=17, we would find the highest multiple of 5 less than or equal to 55, which of course is 55. Then we'd add remainder(17/3) == 2 multiples of L, or 2 * 5, to get 65.

The upshot of this is that you can find your result directly, with just a few integer operations, no looping required.


Basic calculator in Python I am a beginner in Python, I want to know if this code can be improved. Is it possible to make it work without the if/else?

number1=int(input('Enter first number: '))
number2=int(input('Enter Second number: '))
operation=input("Enter operation\n[+,-,*,/]")
if operation == '+':
    sum=number1+number2
elif operation == '-':
    sum =number1-number2
elif operation == '*':
    sum =number1*number2
else:
    sum =number1//number2
print(sum)

If you don't want any if statements then you can do the same with the operator library and a dictionary. This is by using the dict.get method that allows you to have a default value (operator.floordiv) if the value isn't in the dictionary. After getting a valid function from the dictionary, you then just have to call the function.

import operator

operators = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul
}

number1 = int(input('Enter first number: '))
number2 = int(input('Enter Second number: '))
operation = input("Enter operation\n[+,-,*,/]")
print(operators.get(operation, operator.floordiv)(number1, number2))

I would however not assume division if the user enters random data. And so I'd add an if and else. In this case I'd check that the operator is in the operators dictionary, and perform the operation if it is. If it isn't, then I'd tell the user that they entered an invalid operator:

operators = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.floordiv
}

if operation in operators:
    print(operators[operation](number1, number2))
else:
    print('Invalid operator')

You can also make the operator list in the input automatically created from the dictionary too. To create the string '+,-,*,/' you can use ','.join(['+', '-', '*', '/']), to get the array you can then use operators.keys(). Finally you'd have to insert it into the rest of the string, and so I'd use str.format.

operation = input("Enter operation\n[{}]".format(','.join(operators.keys())))

And so tying this all together you could use:

import operator

operators = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.floordiv
}

number1 = int(input('Enter first number: '))
number2 = int(input('Enter Second number: '))
operation = input("Enter operation\n[{}]".format(','.join(operators.keys())))
if operation in operators:
    print(operators[operation](number1, number2))
else:
    print('Invalid operator')


Check if a binary tree is a subtree of another tree I am doing a refresher on algorithms.
I tried to solve the problem of determining if you have 2 trees (T1 and T2), one is a subtree of the other.

I came up with the following:

public boolean isSubtree(Node n1, Node n2){

   if(n2 == null) //Empty Subtree is accepted
      return true;

   if(n1 == null)
      return false;

   //If roots are equal, check subtrees
   if(n1.data == n2.data){
       return isSubTree(n1.left, n2.left) && isSubTree(n1.right, n2.right);
   }
   else{//No match found for this root. Check subtrees
       return isSubTree(n1.left, n2) || isSubTree(n1.right, n2);
   }

}

I think it is correct.
Just to clarify n1 is the root of the tree and n2 the root of the subtree we search for.
I was wondering if someone could review it please? Besides correctness the following trouble me:

How would I calculate its complexity?
Also by Googling I found code for this which was similar to mine but not exactly the same.
For example the code fragments I found had if(n1 == null) return false; while I also check if the n2 is not null.
Mu rationale is that an empty tree and an empty subtree should return true.
But either my logic is wrong or the code posts I found on the internet usually ignore this case.

Any input is highly appreciated

The approach is fundamentally flawed. If you're going to do it this way, you need two methods:

public boolean equals(Node n1, Node n2) {
    if (n1 == n2) return true;
    if (n1 == null || n2 == null) return false;
    if (n1.data != n2.data) return false; // Should use .equals if Node.data isn't primitive
    return equals(n1.left, n2.left) && equals(n1.right, n2.right);
}

public boolean isSubtree(Node n1, Node n2) {
    if (n2 == null) return true;
    if (n1 == null) return false;
    return equals(n1, n2) || isSubtree(n1.left, n2) || isSubtree(n1.right, n2);
}

If you're worried about complexity, you might want to consider whether you can flatten your tree into a canonical (wlog) prefix-order string and use an advanced string matching algorithm.


Creating models manually in MVC I am creating my models manually. I have five tables in my database as follows:

Members
MemberTypeMasters
Payments
Relationships
StatusMasters

My database looks like this:

So far I have 6 files containing auto-implemented properties as below:

Models
 |-- DBCS.cs
 |-- Member.cs
 |-- MemberTypeMaster.cs
 |-- Payment.cs
 |-- Relationship.cs
 |-- StatusMaster.cs

DBCS.cs

using System;
using System.Data.Entity;

namespace Munim_File_Sharing.Models
{
    public class DBCS : DbContext
    {
        public DbSet<Member> Members { get; set; }
        public DbSet<MemberTypeMaster> MemberTypeMasters { get; set; }
        public DbSet<Payment> Payments { get; set; }
        public DbSet<Relationship> RelationShips { get; set; }
        public DbSet<StatusMaster> StatusMasters { get; set; }
    }
}

Member.cs

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace Munim_File_Sharing.Models
{
    public class Member
    {
        [Key]
        public int MemberID { get; set; }
        public string Name { get; set; }
        public string Password { get; set; }
        public string MobileNo { get; set; }
        public string FileName { get; set; }
        public Nullable<DateTime> MemberSince { get; set; }
        public int MemberTypeID { get; set; }

        public virtual MemberTypeMaster MemberTypeMaster { get; set; }
        public virtual ICollection<Payment> Payments { get; set; }
        public virtual ICollection<Relationship> RelationshipSenders { get; set; }
        public virtual ICollection<Relationship> RelationshipReceivers { get; set; }
    }
}

MemberTypeMaster.cs

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace Munim_File_Sharing.Models
{
    public class MemberTypeMaster
    {
        [Key]
        public int MemberTypeID { get; set; }
        public string MemberType { get; set; }

        public ICollection<Member> Members { get; set; }
    }
}

Payment.cs

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace Munim_File_Sharing.Models
{
    public class Payment
    {
        [Key]
        public int PaymentID { get; set; }
        public decimal Amount { get; set; }
        public DateTime DateOfReceipt { get; set; }
        public int MemberID { get; set; }

        public virtual Member Member { get; set; }
    }

}

Relationship.cs

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace Munim_File_Sharing.Models
{
    public class Relationship
    {
        [Key]
        public int RelationshipID { get; set; }
        public int RequestSenderID { get; set; }
        public int RequestReceiverID { get; set; }
        public int StatusID { get; set; }

        public virtual Member RequestSender { get; set; }
        public virtual Member RequestReciever { get; set; }
        public virtual StatusMaster Status { get; set; }
    }
}

StatusMaster.cs

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace Munim_File_Sharing.Models
{
    public class StatusMaster
    {
        [Key]
        public int StatusID { get; set; }
        public string Status { get; set; }

        public virtual ICollection<Relationship> Relationships { get; set; }
    }
}

connectionString in web.Config:

<connectionStrings>
    <add name="DBCS" connectionString="server=.; database=FileSharingDB; Integrated Security = SSPI" providerName="System.Data.SqlClient" />
</connectionStrings>

Is there any problem in my above classes? Am I forgetting anything in Models?

The above classes are used by entity framework for persistance, don't confuse these with the Models in MVC. While its possible to use them directly in MVC, its considered bad practise to so tightly couple your presentation layer to your persistance layer.

Look at tools like AutoMapper that can help you map from the above classes to your actual View Models. Try and have one view model per page and populate them with properties that make sense to the presentation of the data, not its storage.

ie:

public class User
{
    [Key]
    public int ID { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
}

public class UserModel
    {
        public string Name { get; set; }
    }

Mapper.CreateMap<User,UsersModel>()
    .ForMember(dst => dst.Name, m => m.MapFrom(src => src.FirstName + " " + src.LastName))

and

var userModel = Mapper.Map<User, UserModel>(user);

Then use the userModel in your view.


Insertion sort C++14 implementation I have implemented insertion sort using C++. Is there any way to improve my code and is it CppCoreGuideline compliant? As I passed the vector as const in print method, do I need to specify const in range for loop?

/*
 *  Insertion Sort
 */

#include <iostream>
#include <vector>

void insertionSort(std::vector<int> &v);
void print(const std::vector<int> &v);

int main()
{
    std::ios_base::sync_with_stdio(false);

    std::vector<int> v {10, 9, 8, 7, 6, 5, 4, 3, 2, 1};

    // Print the vector before sorting.
    print(v);
    // Sort
    insertionSort(v);
    // Print the vector after sorting.
    print(v);

    return 0;
}

void insertionSort(std::vector<int> &v) {
    for (auto i = v.begin() + 1; i != v.end(); ++i) {
        int key = *i;

        auto j = i - 1;
        while (j >= v.begin() && *j > key) {
            *(j+1) = *j;
            --j;
        }
        *(j+1) = key;
    }
    return;
}

void print(const std::vector<int> &v) {
    for (const auto &i : v) {
        std::cout << i << " ";
    }
    std::cout << "\n";
}

Advice 1

In your insertionSort you can remove the last statement (return).

Advice 2

Making your insertion sort generic is easy:

template<typename RandIt>
void insertion_sort(RandIt begin, RandIt end)
{
    for (auto i = begin + 1; i != end; ++i)
    {
        auto key = *i;
        auto j = i - 1;

        while (j >= begin and *j > key)
        {
            *(j + 1) = *j;
            --j;
        }

        *(j + 1) = key;
    }
}

template<typename Container>
void insertion_sort(Container& cont)
{
    insertion_sort(std::begin(cont), std::end(cont));
}

Advice 3

Making your print function generic is not hard either:

template<typename RandIt>
void print(RandIt begin, RandIt end)
{
    using value_type = typename std::iterator_traits<RandIt>::value_type;
    std::copy(begin, end, std::ostream_iterator<value_type>(std::cout, ", "));
    std::cout << std::endl;
}

template<typename Container>
void print(const Container& container)
{
    print(std::begin(container), std::end(container));
}

Hope that helps.


Check if a stack of Integers contains a sequence of consecutive values Write a method isConsecutive that takes a stack of integers as a
  parameter and that returns whether or not the stack contains a
  sequence of consecutive integers starting from the bottom of the stack
  (returning true if it does, returning false if it does not).
  Consecutive integers are integers that come one after the other, as in
  5, 6, 7, 8, 9, etc. So if a stack s stores the following values:

  bottom [3, 4, 5, 6, 7, 8, 9, 10] top

  Then the call of isConsecutive(s) should return true. If the stack had
  instead contained this set of values:

  bottom [3, 4, 5, 6, 7, 8, 9, 10, 12] top

  Then the call should return false because the numbers 10 and 12 are
  not consecutive. Notice that we look at the numbers starting at the
  bottom of the stack.

  The following sequence of values would be consecutive except for the
  fact that it appears in reverse order, so the method would return
  false: bottom [3, 2, 1] top

  Your method must restore the stack so that it stores the same sequence
  of values after the call as it did before. Any stack with fewer than
  two values should be considered to be a list of consecutive integers.
  You may use one queue as auxiliary storage to solve this problem.

I'm looking for ways to improve my future code. I was also wondering if it's better practice to use generics in cases like these? I'll also accept other methods of solving this problem as the correct answer. My main goal is to try to take away something that will make me a better programmer; if you have advice to shed, please don't hesitate to share it!

public boolean isConsecutive(Stack<Integer> s) {
    boolean consecutive = true;
    Queue<Integer> q = new LinkedList<Integer>();
    while (!s.isEmpty()) {
        q.offer(s.pop());
    }

    while (!q.isEmpty()) {
        s.push(q.poll());
    }

    while (!s.isEmpty()) {
        q.offer(s.pop());
    }

    while (q.size() > 1) {
        int first = q.poll();
        int second = q.poll();
        consecutive &= (second - first == 1);
        s.push(first);
        s.push(second);
    }

    if (!q.isEmpty()) {
        int first = q.poll();
        int second = s.peek();
        consecutive &= (second - first == 1);
        s.push(first);
    }
    return consecutive;
}

Bug 1

Stack<Integer> stack = new Stack<>();
stack.push(1);
stack.push(2);
stack.push(3);
System.out.println(isConsecutive(stack));

returns false.

Bug 2

Your method throws EmptyStackException on stack of size 1.

Advice

I suggest you declare your method as static. That way the client programmer does not need to instantiate an object.

Alternative implementation

This is just a slight modification of my answer to your previous stack related question:

public static boolean isConsecutiveV2(Stack<Integer> stack) {
    if (stack.size() <= 1) {
        return true;
    }

    Stack<Integer> aux = new Stack<>();

    aux.push(stack.pop());

    while (!stack.isEmpty() && stack.peek() == aux.peek() - 1) {
        aux.push(stack.pop());
    }

    boolean consecutive = stack.isEmpty();

    // Restore the input stack:
    while (!aux.isEmpty()) {
        stack.push(aux.pop());
    }

    return consecutive;
}


Find sum of K largest elements in an array The problem statement is to write code that returns the sum of the K largest elements in a given array. The array is not sorted already.

My solution is based on the idea of quicksort, by finding pivot elements until the pivot is actually the kth element. Thus, all elements left of the pivot will be larger than the pivot.

public class Solution {

    public int getKthSum(int[] a, int k) {
        int kthIdx = getKthLargestIndex(a, k);
        int sum = 0;
        for (int i = 0; i <= kthIdx; i ++) {
            sum += a[i];
        }
        return sum;
    }

    private int getKthLargestIndex(int[] a, int k) {
        return getKthLargestIndex(a, k, 0 , a.length - 1);
    }

    private int getKthLargestIndex(int[] a, int k, int l, int h) {
        int p = partition(a, l, h);
        int rank = p - l +1;
        if (rank == k) {
            return p;
        }
        else if ( k < rank) {
            return getKthLargestIndex(a, k, l, p - 1);
        }
        else {
            return getKthLargestIndex(a, k - rank, p +1 , h);
        }
    }

    private int partition(int[] a, int l, int h) {
        int p; //pivot;
        int firstHigh;

        p = h;
        firstHigh = l;
        for (int i = l ; i < h ; i ++) {
            if (a[i] > a[p]) {
                swap(a, i, firstHigh);
                firstHigh++;
            }
        }
        swap(a, p, firstHigh);
        return firstHigh;
    }

    private void swap(int[] a, int i, int j) {
        int temp = a[i];
        a[i] = a[j];
        a[j] = temp;
    }

    public static void main(String[] args) {
        Solution s = new Solution();
        int[] nums = {2,1};
        System.out.println(s.getKthSum(nums, 1));
        System.out.println(Arrays.toString(nums));
        int[] nums2 = {3,2,1,5,6,4};
        System.out.println(s.getKthSum(nums2, 2));
        System.out.println(Arrays.toString(nums2));

    }

Based on my testing, I believe that this implementation works. Is it more or less efficient compared to a maxHeap implementation?

While the idea is perfectly sound, there are reasons to prefer a min heap approach:

The min heap guarantees an \$O(n \log k)\$ time complexity. Your method
may degenerate to \$O(nk)\$ for nearly sorted data.
The min heap works with streaming data. Your method requires the
entire data set to be present.
The min heap preserves the original order. Your method rearranges
data.

You may also gain some performance with tail recursion elimination:

First, convert the getKthLargestIndex into really tail recursive form:

    if (k == rank) {
        return p;
    if ( k < rank) {
        h = p - 1;
    }
    else {
        k -= rank;
        l = p + 1;
    }
    return getKthLargestIndex(a, k, l , h);

Now replace the recursion with a loop:

    private int getKthLargestIndex(int[] a, int k, int l, int h) {
        while (1) {
            int p = partition(a, l, h);
            int rank = p - l +1;

            if (rank == k) {
                return p;
            }

            if ( k < rank) {
                h = p - 1;
            }
            else {
                k -= rank;
                l = p + 1;
            }
        }
    }


Reduce Sensor Messaging Noise to MQTT I'm running MicroPython on an ESP32 relaying sensor data to my MQTT server for Home Assistant. I only want it to emit a message when state has changed and a motion detected state to hold for a minute before returning to a clear state. I see a lot of examples using sleep, but I don't like the blocking nature of sleep as I will be adding more sensors. Instead I've been using ticks_ms() and ticks_diff() to keep the state from fluttering on/off too much, but I can't help but think there's a better/more elegant way to do this that I'm not seeing.
There's some repetition and nesting that sticks out to me
from umqtt.robust import MQTTClient
from machine import Pin, unique_id
from time import ticks_diff, ticks_ms
from ubinascii import hexlify

#Config
MQTT_SERVER = "X.X.X.X"
MQTT_PORT = 1883
MQTT_USER = b"USER"
MQTT_PASSWORD = b"PASSWORD"
MQTT_CLIENT_ID = hexlify(unique_id())
MQTT_TOPIC = b"esp/motion"
mqtt = MQTTClient(MQTT_CLIENT_ID, MQTT_SERVER, MQTT_PORT, MQTT_USER, MQTT_PASSWORD)

ledPin = Pin(2, Pin.OUT)
motPin = Pin(15, Pin.IN)
previousState = 0
delay_ms = 60000
clock = ticks_ms()

def main():
    global clock, previousState, delay_ms

    try:
        mqtt.connect()

        while True:
            state = motPin.value()

            if state == 1:
                ledPin.on()

                if previousState == 0:
                    if ticks_diff(ticks_ms(), clock) >= 0:
                        print('motion_start')
                        mqtt.publish(MQTT_TOPIC, 'motion_start')
                        clock = ticks_ms() + delay_ms
                        previousState = state
                else:
                    clock = ticks_ms() + delay_ms

            else:
                ledPin.off()

                if previousState == 1:
                    if ticks_diff(ticks_ms(), clock) >= 0:
                        print('motion_stop')
                        mqtt.publish(MQTT_TOPIC, 'motion_stop')
                        previousState = state

    finally:
        ledPin.off()
        mqtt.publish(MQTT_TOPIC, 'motion_stop')
        mqtt.disconnect()

if __name__ == "__main__":
    main()

Fluttering
            if previousState == 0:
                if ticks_diff(ticks_ms(), clock) >= 0:

This is called a soft debounce. It may or may not be adequate. My usual recommendation is to do hard debounce instead (or maybe in addition) via a first-order RC lowpass filter followed by hysteresis in a Schmitt trigger. If none of that makes sense to you, then that's a question for https://electronics.stackexchange.com/ .
Truthiness
I would sooner write
if state:

rather than
if state == 1:

snake_case
Per PEP8, previousState should be previous_state.


PHP function for encrypt / decrypt using openssl This is a function that is the building block of one wordpress plugin. I came up with this code after some search but i need to optimize it and get any info from more experienced guys.

public static function afz_encrypt_decrypt( $action, $string ) {
    $output = false;

    $encrypt_method = "AES-256-CBC";
    $secret_key     = '12345678901234567890123456789012';
    $iv_size        = 16;

    if ( $action == 'encrypt' ) {
        $encrypt_iv = mcrypt_create_iv( $iv_size, MCRYPT_DEV_URANDOM );
        $output     = openssl_encrypt( $string, $encrypt_method, $secret_key, 0, $encrypt_iv );
        $output     = base64_encode( $encrypt_iv . $output );
    } else if ( $action == 'decrypt' ) {
        $decrypt_iv = substr( base64_decode( $string ), 0, $iv_size );
        $output     = stripslashes( openssl_decrypt( substr( base64_decode( $string ), $iv_size ), $encrypt_method,
            $secret_key,
            0,
            $decrypt_iv ) );
        if ( false === $output ) {
            $output = $string;
        }
    }

    return $output;
}

Update: The stripslashes is indeed unneeded and i will take it out of context of this function. I need it because wordpress automatically escapes strings regardless if the relevant PHP Setting. See the NOTES section here: CODEX Link

What i still need though is more insight (if possible) to Tim's answer in regards to the security aspect of the source.

I haven't looked at the security aspect of this, so you might want to wait for answers on that aspect.

Function Design

Your function does too many things (encryption and decryption), making it harder to read than necessary and hard to use.

At a minimum you would need to document it properly, as otherwise, users would need to look at the actual code to use it, which isn't ideal.

But really, it will be much simpler to have two functions:

function afz_encrypt($plaintext) {}
function afz_dencrypt($ciphertext) {}

If you don't want to duplicate the encryption parameters, saving those in the class may be a good idea.

I would also pass the key to the function instead of hardcoding it in the function itself.

Return values and error handling

The return values do not seem that well defined. The function returns:

false if an invalid action is given
false if it can't base64 encode on encryption
the base64 encoded iv if it can't encrypt (meaning the input is lost).
the encrypted input string if it can't decrypt (and this happens without any warnings by PHP).

There are probably more irregularities, but I think the point is clear: A user cannot figure out what exactly went wrong, and sometimes cannot even tell that something went wrong, which will sooner or later lead to bugs.

You should throw exceptions on invalid input stating what exactly went wrong (eg wrong action, cannot base64 decode, etc), and check the output of all the functions you use.

Invalid Output

why stripslashes? It means that decrypt(encrypt(plain)) != plain in some situations (eg foo'bar, a\b, etc).
Also, decrypt(encrypt([empty string])) != [empty string] and decrypt(aaaaaaaaaaaaaaaaaaaaaaa) = aaaaaaaaaaaaaaaaaaaaaaa because of the issues outlined above.


JQuery Clyp Auto-embed This function finds links from Clyp.it and offers users a link to embed the content. Please let me know what you think of it. It works, but probably has some redundancies and inefficiencies.

$(document).ready(function() {
    var clyplinks = $('a[href^="https://clyp.it/"]').filter(function() {
        return this.innerHTML.match(/(https:\/\/clyp.it\/)([\w]{8})/);
    });
    $(clyplinks).each(function(index) {

        var x = null;
        var id = $(this).attr("href").split("it/")[1];
        var e = $(' [embed]').insertAfter($(this));
        e.click(function(evt) {
            evt.preventDefault();
            if (x != 1) {
                $(this).after('

')
                $(e).html(' [remove]')
                x = 1;
                console.log('embed' + x + ' ' + index);
            } else {
                $('#clyp' + id + index).detach();
                $(e).html(' [embed]')
                x = null;
                console.log('yay' + x + ' ' + index);

            }

        });

    });
});

https://clyp.it/5e0dmibw

JSFiddle:

https://jsfiddle.net/1svgq6g4/

Regex

The regex used is /(https:\/\/clyp.it\/)([\w]{8})/.
Since, here you're checking if the string is URL and not using to extract data from string, capturing groups can be removed.

[\w]: Match word character(alphanumeric character and underscore). This can be written as \w by removing character class.

I'll also suggest to use word boundary and use of i-case-insensitive flag to match both uppercase and/or lowercase alphabets.

/\bhttps:\/\/clyp.it\/\w{8}\b/

match vs test

String#match is used to extract data from string. To check if string follows particular pattern, use RegExp#test. test is faster than match.

See regex.test V.S. string.match to know if a string matches a regular expression

$('a[href^="https://clyp.it/"]').filter(function() {
    return /https:\/\/clyp.it\/\w{8}/i.test(this.innerHTML);
});

Variable Naming

Variable names should be self-descriptive. I'll recommend to read https://github.com/ryanmcdermott/clean-code-javascript#variables.

clyplinks can be just links or clypLinks(camelCase)
x can be renamed to flag and used as boolean(true/false)
e should be el if you like short names or element. As it is jQuery wrapped element, it is common to use $ as prefix in variable name- $element.

Event binding

For single link, this will work. If there are multiple links on the page to bind events on dynamic elements event delegation can be used. When using event delegation, the code should be moved outside of each so that event is not bound multiple times.

The syntax is

$(parentElementSelector).on(event, childElements, callbackFunction)

In this case, as there is no common parent element which contain all the links, we can use document

$(document).on('click', '.embed', function(event) {

Using data-*

Instead of using a flag to store the status of link, HTML5 custom data-* can be used. So, on click of link, first check if variable value is set and depending on the value perform action. To read the value of data-* attribute use data().

// Inside click handler
if ($(this).data('state') === 'embedded') {
    // Already embedded
    // Remove
} else {
    // Not embedded
    // Add
}

// Change the value
$(this).data('state', function(i, value) {
    // switch the value
    return value === 'embedded' ? 'not embedded' : 'embedded';
});

After doing these changes, previous code will not work. Inside the handler $(e).html( is used to change the link text. Note that html() will set the innerHTML of the element and not replace it. If you inspect the Embed link by clicking on it, you'll notice that link is nested. To solve this, just use $(this).html('[remove]'). Also, note that $(this) inside event handler refer to the element on which the event has occurred.

jQuery Wrapping & Method Chaining

clyplinks is already jQuery collection of element. There is no need of wrapping it in jQuery again.

jQuery allows to chain most of the methods.

var clyplinks = $('a[href^="https://clyp.it/"]').filter(function() {
    return this.innerHTML.match(/(https:\/\/clyp.it\/)([\w]{8})/);
});
$(clyplinks).each(function(index) {

can be written as

$(someSelector).filter(function() {
    return ...;
}).each(function(index) {

This also eliminates clyplinks variable.

Below things are also important

Code Formatting: You may install plugin/package in editor/IDE
Missing Semi-colons: Use jslint/jshint


Count PDF pages in constructor I have an object that represents a PDF file. In the constructor, I pull out various information about the file name and make it available via properties:

public class Invoice
{
    //public properties
    public string FullPath {get { return this.fullPath;} }
    public string FileNameWithoutExtension { get { return this.fileNameWithoutExtension;} }
    public string FileName { get { return this.fileName; } }
    public string BatchSequenceNumber { get { return this.batchSequenceNumber; } }

    //private fields
    private string fullPath;
    private string fileNameWithoutExtension;
    private string fileName;
    private string batchSequenceNumber;

    //Default construtor
    public Invoice(string filePath)
    {
        this.fullPath = filePath;
        this.fileName = Path.GetFileName(filePath);
        this.fileNameWithoutExtension = Path.GetFileNameWithoutExtension(filePath);
        this.batchSequenceNumber = Path.GetFileNameWithoutExtension(filePath).Split('_').LastOrDefault();
    }
}

One of the attributes I would like to get is the number of pages in the PDF.
I am using iTextSharp and I have a PageCount method in the above Invoice class that looks like this:

public int PageCount()
    {
        PdfReader reader = null;
        int pageCount;

        try
        {
            reader = new PdfReader(this.FullPath);
            pageCount = reader.NumberOfPages;
        }
        catch (Exception ex)
        {
            throw new Exception("Error reading pdf! " + ex.Message);
        }
        finally
        {
            if (reader != null) { reader.Close(); }
        }

        return pageCount;
    }

This works fine, except now I need to get the count of pages at several times during the object lifetime. I do not want to keep opening the PDF each time to get the count so these are my ideas:

Rename method PageCount to CountPages and store the result in field/property PageCount.
Count the pages in the constructor and store the result in a field/propertry PageCount and don't expose a CountPages method.

Number 1 seems preferable to me, but what if someone tries to read PageCount before running method CountPages? How best to handle this situation, set PageCount to 0 in the constructor?

Number 2 seems bad because it feels wrong to open the PDF file in the constructor, is it? (I have been reading this)

Assuming you want to avoid opening the file unless necessary (ie until someone requests the PageCount), I would use a nullable private field, as follows:

private int? _pageCount;

public int PageCount
{
    get
    {
        if (!_pageCount.HasValue) {
            //existing code to determine page count

            _pageCount = /* result */
        }
        return _pageCount;
    }
}

The first time the property is accessed, the private member will be null, so the code to update the value will be run.  Subsequent accesses will use the cached value.


Change type of property in derived class I'm using manually implemented properties to handle sharing code between forms intended to edit a base object, and those intended to edit a derived version.  My current setup works as long as everyone accesses the property and not the base object; but after spending an hour or so tracking down a bug that was due to manipulating _editDataObject in DataObjectEditForm in an application using DataObjectEx and DataObjectExEditForm I'm wondering if there's a way to enforce this more strongly than just adding a do not use comment on _editDataObject.

For contractual reasons the simple option of creating a single form that is aware of both DataObject and DataObjectEx isn't permissible.

public class DataObject
{
    //data members go here
}

public class DataObjectEx : DataObject
{
    public DataObjectEx(DataObject dataObject)
    {
         //create DataObjectEx containing all of dataObject's state.
    }

    //additional data members go here
}

public class DataOjectEditForm : Form
{
    ///<Summary>
    ///Do not use.  Will break app if EditDataObject property is overridden.
    ///</Summary>
    private _editDataObject;

    protected virtual EditDataObject
    {
        get { return _editDataObject; }
        set { _editDataObject = value; }
    }

    //Do stuff only dependent on DataObject
}

public class DataOjectExEditForm : DataObjectEditForm
{
    private _editDataObjectEx;

    protected override EditDataObject
    {
        get { return _editDataObjectEx; }
        set
        {
            if (value == null)
               _editDataObjectEx = null;
            else _editDataObjectEx = value as DataObjectEx ?? new DataObjectEx(value);
        }
    }

    //Add stuff dependent on the additions in DataObjectEx
}

Judging from the code you provided, the program is behaving as if you are using the new keyword.  This causes your property to behave differently based upon whether your form is used in the context of type DataObjectEditForm versus type DataObjectExEditForm.

To clarify:

var form = new DataObjectExEditForm ();
form.EditDataObject = new DataObjectEx ();

DataObjectEditForm  baseForm = form;
var  data = baseForm.EditDataObject;  // data = null!?!

From the looks of it, you should instead be using virtual/override on your property.

public class DataOjectEditForm : Form
{
    protected DataObject _editDataObject;

    protected virtual DataObject EditDataObject
    {
        get { return _editDataObject; }
        set { _editDataObject = value; }
    }

    //Do stuff only dependent on DataObject
}

public class DataOjectExEditForm : DataObjectEditForm
{
    protected override DataObject EditDataObject
    {
        get { return _editDataObject; }
        set
        {
            if (value == null)
               _editDataObject = null;
            else _editDataObject = value as DataObjectEx ?? new DataObjectEx(value);
        }
    }

    //Add stuff dependent on the additions in DataObjectEx
}

Of course, that is only going to guarantee that the backing field used by the form is consistent.  If the bug is the result of _editDataObject being used in some contexts while _editDataObjectEx was used in other contexts, then this will solve your issue.


"Laser treatment" SPOJ challenge I am trying to solve this problem on SPOJ Brazil. I'm trying to approach the problem with a series of logical conditions and my code looks like this and it works perfectly.

#include <stdio.h>

typedef struct {
    int y1;
    int x1;
    int x2;
    int y2;
}coordinate;

int main(){

    int N, C, i, j, xi, xf, nbacterias;

    coordinate coord[100000];

    scanf ("%d %d", &N, &C);

    for (i = 0; i < N; i++){
        scanf ("%d %d %d %d", &coord[i].x1, &coord[i].y1, &coord[i].x2, &coord[i].y2);
    }

    for (i = 0; i < C; i++){
        scanf ("%d %d", &xi, &xf);
        nbacterias = 0;

        for (j = 0; j< N; j++){
            if ((coord[j].x1 >= xf && coord[j].x2 >= xf) || (coord[j].x2 <= xi && coord[j].x1 <= xi))
                nbacterias++;
            else if ((xf < coord[j].x2 && xf > coord[j].x1 && xi <= coord[j].x1 && xi < coord[j].x2) || (xf < coord[j].x1 && xf > coord[j].x2 && xi <= coord[j].x2 && xi < coord[j].x1))
                nbacterias++;
            else if ((xi > coord[j].x2 && xi < coord[j].x1 && xf >= coord[j].x1 && xf > coord[j].x2) || (xi>coord[j].x1 && xi < coord[j].x2 && xf >= coord[j].x2 && xf > coord[j].x1))
                nbacterias++;
            else if ((coord[j].x1 < xi && coord[j].x2 > xf) || (coord[j].x2 < xi && coord[j].x1 > xf))
                nbacterias = nbacterias + 2;

        }
        printf ("%d\n", nbacterias);
    }

    return 0;
}

As you can see, I created a coordinate struct and created a series of conditions in which the bacteria could be divided (in two, when they get hit by the laser in the middle; in one, when the laser cuts out a part of it; when the laser doesn't hit the bacteria at all, leaving the whole bacteria; and when the laser hits the entire bacteria, leaving no parts at all).

This solution works, you can test it with the cases given in the problem link I pasted up there. The problem is that when I submit my code it exceeds the time limit. Any ideas on how to optimize this code or if I am missing some gimmick here? By the way, I'm pretty new to competitive programming, so I may not completely grasp complex answers.

Style

Your code looks nice. It is not splitted into small functions and it is not commented but it's easy to understand anyway.

A few easy things could be done to make it more beautiful :

define variables in the smallest possible scope. With the right version of C, this also include defining variables as part of the for syntax. This makes the code much clearer as it is easier to see where the variable is used.
define a variable to refer to coord[j].
use += instead of repeating variable = variable + something.

At this stage, the code looks like :

int main(){
    coordinate coord[100000];

    int N, C;
    scanf ("%d %d", &N, &C);

    for (int i = 0; i < N; i++){
        scanf ("%d %d %d %d", &coord[i].x1, &coord[i].y1, &coord[i].x2, &coord[i].y2);
    }

    for (int i = 0; i < C; i++){
        int xi, xf;
        scanf ("%d %d", &xi, &xf);
        int nbacterias = 0;

        for (int j = 0; j< N; j++){
            coordinate c = coord[j];
            if ((c.x1 >= xf && c.x2 >= xf) || (c.x2 <= xi && c.x1 <= xi))
                nbacterias++;
            else if ((xf < c.x2 && xf > c.x1 && xi <= c.x1 && xi < c.x2) || (xf < c.x1 && xf > c.x2 && xi <= c.x2 && xi < c.x1))
                nbacterias++;
            else if ((xi > c.x2 && xi < c.x1 && xf >= c.x1 && xf > c.x2) || (xi>c.x1 && xi < c.x2 && xf >= c.x2 && xf > c.x1))
                nbacterias++;
            else if ((c.x1 < xi && c.x2 > xf) || (c.x2 < xi && c.x1 > xf))
                nbacterias+=2;

        }
        printf ("%d\n", nbacterias);
    }

    return 0;
}

Tests

Before trying to make things faster, it is a good idea to write automatic tests. At the moment, your code relies on input, output so it is not really easy to tests. It could be nice to separate the concerns: the input/output on one hand, the logic on the other hand.

Let's write a function to perform the logic. Now, we can reuse data from the problem to write a few tests :

#include <stdio.h>
#include <assert.h>

typedef struct {
    int x1;
    int y1;
    int x2;
    int y2;
}coordinate;

int nb_bacteria_after_beam(coordinate coord[], int N, int xi, int xf)
{
    int nbacterias = 0;

    for (int j = 0; j< N; j++){
        coordinate c = coord[j];
        if ((c.x1 >= xf && c.x2 >= xf) || (c.x2 <= xi && c.x1 <= xi))
            nbacterias++;
        else if ((xf < c.x2 && xf > c.x1 && xi <= c.x1 && xi < c.x2) || (xf < c.x1 && xf > c.x2 && xi <= c.x2 && xi < c.x1))
            nbacterias++;
        else if ((xi > c.x2 && xi < c.x1 && xf >= c.x1 && xf > c.x2) || (xi>c.x1 && xi < c.x2 && xf >= c.x2 && xf > c.x1))
            nbacterias++;
        else if ((c.x1 < xi && c.x2 > xf) || (c.x2 < xi && c.x1 > xf))
            nbacterias+=2;
    }
    return nbacterias;
}

int automatic_tests()
{
    coordinate coord[] = {{1, 0, 4, 0}};
    assert(nb_bacteria_after_beam(coord, 1, 0, 2) == 1);
    assert(nb_bacteria_after_beam(coord, 1, 3, 5) == 1);
    assert(nb_bacteria_after_beam(coord, 1, 2, 3) == 2);
    assert(nb_bacteria_after_beam(coord, 1, 0, 5) == 0);
    coordinate coord2[] = {{2, 0, 4, 0}, {2, 0, 4, 0}};
    assert(nb_bacteria_after_beam(coord2, 2, 0, 1) == 2);
    assert(nb_bacteria_after_beam(coord2, 2, 1, 2) == 2);
    assert(nb_bacteria_after_beam(coord2, 2, 2, 3) == 2);
    assert(nb_bacteria_after_beam(coord2, 2, 3, 4) == 2);
    assert(nb_bacteria_after_beam(coord2, 2, 4, 5) == 2);
    assert(nb_bacteria_after_beam(coord2, 2, 5, 6) == 2);
    coordinate coord3[] = {{0, 0, 3, 5}, {3, 5, 0, 2}};
    assert(nb_bacteria_after_beam(coord3, 2, 0, 3) == 0);
    assert(nb_bacteria_after_beam(coord3, 2, 1, 2) == 4);
    assert(nb_bacteria_after_beam(coord3, 2, 2, 7) == 2);
    return 0;
}

int stdin_tests()
{
    coordinate coord[100000];

    int N, C;
    scanf ("%d %d", &N, &C);

    for (int i = 0; i < N; i++){
        scanf ("%d %d %d %d", &coord[i].x1, &coord[i].y1, &coord[i].x2, &coord[i].y2);
    }

    for (int i = 0; i < C; i++){
        int xi, xf;
        scanf ("%d %d", &xi, &xf);
        printf ("%d\n", nb_bacteria_after_beam(coord, N, xi, xf));
    }

    return 0;
}

int main(){
    return automatic_tests();
}

Please note that I had to reorder members of your struct so that I can simply reuse tests cases from the problem.

Now, I assume (which might not be nor true nor wise) that if anything goes wrong during my changes, I'll detect it almost instantly.

Better performances

At the moment, your code works by implementing the functionality with a straight-forward algorithm considering the different cases.

A simple observation can lead to better algorithm. For instance, if we order (x1,y1) and (x2,y2) in such a way that the left-most end is (x1,y2) and the right-most end is (x2,y2) then we have a much smaller number of situations. Assuming xi < xf, we have a bacteria on the left if and only if x1 < xi and a bacteria on the right if and only if x2 > xf (with the 4 combinations possible : right, left, right + left, none).

Before trying to perform any optimisation, we should try to be in a situation where performances can be properly measured : at the moment, the times are much too short to be analysed and the datasets might not correspond to situations leading to timeout.

Before I go further, the assumption here is that your code is not efficient enough when the number of queries performed on a sample gets huge. (This assumption is based on the fact that the values can be much bigger for the number of cases than the number of bactoeria). We can reproduce such a case by putting the lines doing assert(nb_bacteria_after_beam(....)) in for loops. This gives measurable times on my configuration :

int automatic_tests()
{
    int nb_tests = 10000000;
    coordinate coord[] = {{1, 0, 4, 0}};
    coordinate coord2[] = {{2, 0, 4, 0}, {2, 0, 4, 0}};
    coordinate coord3[] = {{0, 0, 3, 5}, {3, 5, 0, 2}};
    for (int i = 0; i < nb_tests; i++)
    {
        assert(nb_bacteria_after_beam(coord, 1, 0, 2) == 1);
        assert(nb_bacteria_after_beam(coord, 1, 3, 5) == 1);
        assert(nb_bacteria_after_beam(coord, 1, 2, 3) == 2);
        assert(nb_bacteria_after_beam(coord, 1, 0, 5) == 0);
        assert(nb_bacteria_after_beam(coord2, 2, 0, 1) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 1, 2) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 2, 3) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 3, 4) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 4, 5) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 5, 6) == 2);
        assert(nb_bacteria_after_beam(coord3, 2, 0, 3) == 0);
        assert(nb_bacteria_after_beam(coord3, 2, 1, 2) == 4);
        assert(nb_bacteria_after_beam(coord3, 2, 2, 7) == 2);
    }
    return 0;
}

On such a dataset, I can already see the benefits of the idea described above. Correspond code looks like :

int nb_bacteria_after_beam(coordinate coord[], int N, int xi, int xf)
{
    int nbacterias = 0;
    assert(xi < xf);

    for (int j = 0; j< N; j++){
        coordinate c = coord[j];
        int x1 = c.x1;
        int x2 = c.x2;
        if (x1 > x2)
        {
            // swap
            int tmp = x1;
            x1 = x2;
            x2 = tmp;
        }
        if (x1 < xi)
            nbacterias++;
        if (x2 > xf)
            nbacterias++;
    }
    return nbacterias;
}

When the number of cases gets huge indeed, it might be worth performing some preprocessing to ensure queries can be performed more efficiently.

We can easily write a procedure to prepare our datasets in such a way that we can apply the trick described above directly.

This leads to much better performances indeed :

void prepare_dataset(coordinate coord[], int N)
{
    for (int j = 0; j< N; j++){
        if (coord[j].x1 > coord[j].x2){
            // swap (to keep things nice, y coord has to be moved too even if it is not useful)
            int tmp = coord[j].x1;
            coord[j].x1 = coord[j].x2;
            coord[j].x2 = tmp;
            tmp = coord[j].y1;
            coord[j].y1 = coord[j].y2;
            coord[j].y2 = tmp;
        }
    }
}

int nb_bacteria_after_beam(coordinate coord[], int N, int xi, int xf)
{
    int nbacterias = 0;
    assert(xi < xf);

    for (int j = 0; j< N; j++){
        coordinate c = coord[j];
        assert(c.x1 <= c.x2);
        if (c.x1 < xi)
            nbacterias++;
        if (c.x2 > xf)
            nbacterias++;
    }
    return nbacterias;
}

int automatic_tests()
{
    int nb_tests = 10000000;
    coordinate coord[] = {{1, 0, 4, 0}};
    prepare_dataset(coord, 1);
    coordinate coord2[] = {{2, 0, 4, 0}, {2, 0, 4, 0}};
    prepare_dataset(coord2, 2);
    coordinate coord3[] = {{0, 0, 3, 5}, {3, 5, 0, 2}};
    prepare_dataset(coord3, 2);
    for (int i = 0; i < nb_tests; i++)
    {
        assert(nb_bacteria_after_beam(coord, 1, 0, 2) == 1);
        assert(nb_bacteria_after_beam(coord, 1, 3, 5) == 1);
        assert(nb_bacteria_after_beam(coord, 1, 2, 3) == 2);
        assert(nb_bacteria_after_beam(coord, 1, 0, 5) == 0);
        assert(nb_bacteria_after_beam(coord2, 2, 0, 1) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 1, 2) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 2, 3) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 3, 4) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 4, 5) == 2);
        assert(nb_bacteria_after_beam(coord2, 2, 5, 6) == 2);
        assert(nb_bacteria_after_beam(coord3, 2, 0, 3) == 0);
        assert(nb_bacteria_after_beam(coord3, 2, 1, 2) == 4);
        assert(nb_bacteria_after_beam(coord3, 2, 2, 7) == 2);
    }
    return 0;
}

More performance

This is pretty good but we haven't really changed the complexity of the algorithm. It used to be 0(N*C) and it still is. Only the constant factor has changed (basically, we still perform a huge number of operation considering all bacteria for all inputs, we just handle them in a more efficient way). What could be interesting (and is probably what is expected by the people who wrote the problems) would be to prepare the dataset in such a way that for a given input, you don't have to reconsider all bacteria again and again.

The point would be to be able to create some kind of summary of your dataset in such a way that you can easily determine the information you need. Basically, it boils down to determine how many items in a list are smaller or bigger than a value you'll be provided later. Indeed, the solution of the problem is the sum of the number of x1 such that x1 < xi and the number of x2 such that x2 > xf. An interesting idea could be to create a sorted array of the x1 values and a sorted array of the x2 values (don't forget that in order to have this trick to work, you first have to re-arrange x1,x2 in such a way that x1 < x2). Once this is done, the number of items smaller (or bigger) than a value can be determined with a binary search algorithm.

Unless I am mistaken, the complexity for such a strategy would be : 0( N*log(N)  +  C * log(N)) because sorting is 0(n*log(n)) and binary search is O(log(n)).

This should be much faster. I'll let you consider this :-)


Radio button selection and flash object initialization Looking for a code review, as working alone on a project and don't have anyone who could possible help me.

if ($('.qp-content-right-scroll-panel').is(':visible')) {

    var isLiveData = $("input:radio[name=preview-options]").val();
    if (type.indexOf('DASHBOARD_LAYOUT') != -1) {
        var base = $('.qp-content-right-scroll-panel .layoutContent');
        var flashObj = $($.browser.mozilla ? "[name=layoutExternalflash]" :
            '#layoutExternalflash', base).get(0);
        if (flashObj != undefined && $.isFunction(flashObj.ext_sendLayoutDefinition)) {
            //get data from radio buttons
            $("input:radio[name=preview-options]").click(function () {
                var isLiveData = $(this).val();
                flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
            });
        }
    } else {
        var base = $('.qp-content-right-scroll-panel .chartContent');
        var flashObj = $($.browser.mozilla ? "[name=chartExternalflash]" :
            '#chartExternalflash', base).get(0);
        if (flashObj != undefined && $.isFunction(flashObj.ext_sendChartDefinition)) {
            $("input:radio[name=preview-options]").click(function () {
                var isLiveData = $(this).val();
                flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
            });
            flashObj.ext_sendChartDefinition(elementId, state, isLiveData);
        }
    }
}

From the looks of it, you are using jQuery as your JavaScript library. Here are the improvements that I think will make your code better.

Don't duplicate code. Use functions (objects).

For example:
flashObj must be accessible to this function.

 var getDataFromButton = function(elementId, state, isLiveData){
   $("input:radio[name=preview-options]").click(function () {
     var isLiveData = $(this).val();
     flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
   });
 }

Use local variables were possible.
Instead of defining the variable 'base' and 'flashObject' twice, define it once outside the if/else statements and modify it inside them.

Or you can redesign the logic by making an object that has properties such as base, isLiveData, and flashObject.

Without knowing the purpose of this code, this is all I can suggest as improvements.


Splitting a command line into key/value pairs The code below will split environment variables from a command line (always appear at the end of the command line). Environment variables are represented by '-E key=value'. I've achieved this like so, but I'm wondering if there's a more elegant way

public class TestSplit {

  public static void main(String... args) {
    String command = "-ps 4 -pe 5 -E opInstallDir=/home/paul -E opWD=/home/paul/remake -E opFam=fam -E opAppli=appli";

    int startPosition = command.indexOf("-E") + 2;

    String envVars = command.substring(startPosition);

    for(String pair: envVars.split("-E")) {
      String[] kv = pair.split("=");
      System.out.println(kv[0] + " " +kv[1]);
    }
  }

}

EDIT Just to clarify these aren't command line arguments for launching the program from the console, they are command line arguments for launching an external program. The details of which I haven't included.

Like @palacsint I will recommend an external library. Apache commons-cli is a decent choice. Another choice (my preference) is java gnu-getopt ... I like it because I am familiar with the notations and standards from previous work. It can be a little complicated the first time around otherwise.

On the other hand, I tend not to use an external library unless the code is already going to be relatively complicated....

But, back to your code.

Why do you have everything in a single String? Why is it not part of the String...args ?

The first thing about command-line arguments is that they get complicated very fast. What if the argument was:

String command = "-ps 4 -pe 5 -E opInstallDir=/opt/OSS-EVAL/thiscode -E opWD=/home/paul/remake -E opFam=fam -E opAppli=appli -Edocs='My Documents' -Eparse=key=value";

I have thrown in a few things there.

First up, on our one machine at work, we really do have the directory /opt/OSS-EVAL/ which we use to install/evaluate OSS software/libraries.

The above will break your parsing because it has the -E embedded in the name.

Next up, is 'POSIX-style' commandline arguments can have quoted values, and also values with an = in the value.

So, things I would recommend to you:

Locate the source of your command-line values. It will likely be available as an array, not a single string. Keep the data as an array!

Second, with the array, it is easier to look for stand-alone values that are -E, or, if the input is -Ekey=value then you look for values that start with -E.

Finally, when you split the key/value on the =, limit the split to 2.

String[] kv = pair.split("=", 2);

Which will preserve any of the = tokens inside the value part.

EDIT:

You have suggested in your edit that this is for sending data to an external command.

If you are using Java to initialize the external command, then please, please, please use the version of exec() that takes a command array, or use the ProcessBuilder which allows you to send all the command-line parameters as separate values in an array!!!


Using LINQ or Lambda instead of nested and multiple foreach statements I want to use a better syntax than nested foreach statements to overwrite the initial list with items from the second list.

In the code below:

I want to overwrite initialList with those in secondList that have the same Value (Remove Red).
Use the items in secondList where Value was the same (Yellow)

New initialList list should include (Green and Yellow).

static void Main(string[] args)
{
    int useProd = 2;
    int useDomain = 0;

    var person1 = new Person() { prodId = 1, Value = "foo", domainId = 0, Name = "Red" };
    var person2 = new Person() { prodId = 1, Value = "bar", domainId = 0, Name = "Green" };
    var person3 = new Person() { prodId = 1, Value = "foo", domainId = 1, Name = "Yellow" };

    var initialList = new List<Person>();
    initialList.Add(person1);
    initialList.Add(person2);

    var secondList = new List<Person>();
    secondList.Add(person3);

    List<Person> personsToRemove = new List<Person>();
    List<Person> personsToUpdate = new List<Person>();

    foreach (var pers1 in initialList)
    {
        foreach (var pers2 in secondList)
        {
            if (pers1.Value == pers2.Value)
            {
                personsToRemove.Add(pers1);
                personsToUpdate.Add(pers2);
            }
        }
    }
    foreach (var remPers in personsToRemove)
    {
        initialList.Remove(remPers);
    }
    foreach (var updPers in personsToUpdate)
    {
        initialList.Add(updPers);
    }
    foreach (var item in initialList)
    {
        Console.WriteLine(String.Format("Value: {0}, prodId: {1}, domainId: {2}, Name: {3}", item.Value, item.prodId, item.domainId, item.Name));
    }

    Console.ReadKey();
}
public class Person
{
    public int prodId { get; set; }
    public string Value { get; set; }
    public int domainId { get; set; }
    public string Name { get; set; }
}

By using an IEqualityComparer<Person> which only compares the Value property like

public class PersonValueComparer : IEqualityComparer<Person>
{
    public bool Equals(Person x, Person y)
    {
        if (x == null && y == null) { return true; }
        if (x == null || y == null) { return false; }

        return x.Value == y.Value;
    }

    public int GetHashCode(Person obj)
    {
        if (obj == null || obj.Value == null) {return 0;}
        return obj.Value.GetHashCode();
    }
}

you can by using linq methods replace the loops by

        PersonValueComparer valueComparer = new PersonValueComparer();
        initialList = secondList.Where(p => initialList.Contains(p, valueComparer))
            .Concat(initialList.Where(p => !secondList.Contains(p, valueComparer))).ToList();

We are first using the Where() method to filter the secondList for items which are in the initialList . Then we use Concat() to add the Person''s which are not in thesecondList`.

Based on the naming guidelines properties should be named using PascalCase casing.

You should always use meaningful and descriptive names for naming methods, variables, properties and classes so Sam the Maintainer sees at first glance what it is about.

Shortening variable names doesn't add any value but instead reduces readability which Sam the Maintainer doesn't like.


LW, MTD, STD, YTD Sales... all in line and looking pretty That's Last Week, Month-to-Date, Season-to-Date, and Year-to-Date sales.

I have a table-valued function for each one, that takes a date and returns all CalendarDate values for the interesting time span:

dbo.CalendarDatesLW
dbo.CalendarDatesMTD
dbo.CalendarDatesSTD
dbo.CalendarDatesYTD

So I have a CTE for selecting YTD sales (where "year" runs from December to November):

with
    cteYTD (SalesRep, Category, Units, Amount) as (
        select
            sales.RepCode,
            sales.Category,
            sum(sales.TotalUnits),
            sum(sales.TotalNetAmount)
        from dbo.Sales sales
            inner join dbo.CalendarDatesYTD(getdate()) calendar on sales.InvoiceDate = calendar.CalendarDate
        group by
            sales.RepCode,
            sales.Category),

..then another for LW sales:

    cteLW (SalesRep, Category, Units, Amount) as (
        select
            sales.RepCode,
            sales.Category,
            sum(sales.TotalUnits),
            sum(sales.TotalNetAmount)
        from dbo.Sales sales
            inner join dbo.CalendarDatesLW(getdate()) lw on sales.InvoiceDate = lw.CalendarDate
        group by
            sales.RepCode,
            sales.Category),

...another for MTD sales:

    cteMTD (SalesRep, Category, Units, Amount) as (
        select
            sales.RepCode,
            sales.Category,
            sum(sales.TotalUnits),
            sum(sales.TotalNetAmount)
        from dbo.Sales sales
            inner join dbo.CalendarDatesMTD(getdate()) mtd on sales.InvoiceDate = mtd.CalendarDate
        group by
            sales.RepCode,
            sales.Category),

...and another for STD sales:

    cteSTD (SalesRep, Category, Units, Amount) as (
        select
            sales.RepCode,
            sales.Category,
            sum(sales.TotalUnits),
            sum(sales.TotalNetAmount)
        from dbo.Sales sales
            inner join dbo.CalendarDatesSTD(getdate()) std on sales.InvoiceDate = std.CalendarDate
        group by
            sales.RepCode,
            sales.Category)

..and they all individually run pretty much instantly (0-1 seconds each, execution plan isn't complaining about any would-be-missing index). Then I join them:

select
    ytd.SalesRep,
    ytd.Category,
    isnull(lw.Units,0) UnitsLW,
    isnull(lw.Amount,0) AmountLW,
    isnull(mtd.Units,0) UnitsMTD,
    isnull(mtd.Amount,0) AmountMTD,
    isnull(std.Units,0) UnitsSTD,
    isnull(std.Amount,0) AmountSTD,
    ytd.Units UnitsYTD,
    ytd.Amount AmountYTD
from cteYTD ytd
    left join cteSTD std on ytd.SalesRep = std.SalesRep
                        and ytd.Category = std.Category
    left join cteMTD mtd on ytd.SalesRep = mtd.SalesRep
                        and ytd.Category = mtd.Category
    left join cteLW lw on ytd.SalesRep = lw.SalesRep
                        and ytd.Category = lw.Category

That's 17 seconds, for 35 rows - and it gets exponentially worse if I start adding more columns in the group by ...and the execution plan looks very, very weird (although not surprising, given the script):

I'm querying dbo.Sales 4 times, grabbing a different time span each time, and then aggregating the same fields over and over. There has to be a better way of doing this... right?

dbo.Sales is a view, joining multiple FK's and implementing some business rules (e.g. such or such customer sales under category X should be reported under category Y).

My gut-feeling is that I'm abusing CTE's and should be using something else to do this. But what?

I think your problem is do with the amount of cartesian products you are forming. I would approach something like this, as I alluded to in chat:

with salesYTD as (
    SELECT SalesPerson, InvoiceDate, Sum(Amount) As AmountOnDay
    FROM sales
    WHERE InvoiceDate > DATEADD(year, DATEDIFF(year, 0, GETDATE()), 0)
    GROUP By SalesPerson, InvoiceDate
),
calculatedYTD as (
    SELECT SalesPerson, SUM(AmountOnDay)
    FROM salesYTD
    GROUP BY SalesPerson
),
calculatedMTD as (
    SELECT SalesPerson, SUM(AmountOnDay)
    FROM salesYTD
    WHERE InvoiceDate >= DATEADD(month, DATEDIFF(month, 0, GETDATE()), 0)
    GROUP BY SalesPerson
)
-- etc
SELECT *
FROM calculatedYTD ytd
LEFT JOIN calculatedMTD mtd ON ytd.SalesPerson = mtd.SalesPerson

I realise the above is not the same code as you posted, trying to show a basic principal, will expand if you want, just shout in comments

But the basic principle is:

Perform the expensive operation once, specifically the one with the biggest date range (YTD)
Perform the rest of the calculation from the result of the first one (its already filtered)

But there are a few things you can do, to which I have made the assumptions above, assuming:

YTD means the beginning of the year
MTD means the beginning of the month... etc...

you can:

Do away with the calculation functions (they are expensive compared to...)
Calculate your YTD as DATEADD(year, DATEDIFF(year, 0, GETDATE()), 0)
Calculate your MTD as DATEADD(month, DATEDIFF(month, 0, GETDATE()), 0)

I haven't run it, but I suspect that it will give you far better IO/performance. Let me know if it helps...


Enable or disable sections of a model according to user input I am using sed command to comment in or out sections in my model according to user input (commenting is done by prefixing the line with !).  Once this is done, I rebuild my model to get a new executable. An example of a model file (named as scm_code_ex.txt in the script) is:

code
code
code-auto
code-auto
code-auto
code
code
code
code
code-agg
code-agg
code-agg
code
code
code
code
code
code
code
code

My script asks the user which sections of the code to comment out (off means comment out):

#!/bin/bash
# This script will comment in or out sections in the code text file
echo "- Aggregation (on) or (off) (type in ans)?"
read agg

echo "- Autoconversion (on) or (off) (type in ans)?"
read auto

#Revert to original with everything ON.

#On autoconversion
    sed -e'3s/\!//' scm_code_ex.txt >scm_code_ex.tmp
    sed -e'4s/\!//' scm_code_ex.tmp >scm_code_ex.tmp2
    sed -e'5s/\!//' scm_code_ex.tmp2 >scm_code_ex.tmp3

#On aggreagation
    sed -e '10s/\!//' scm_code_ex.tmp3 > scm_code_ex.tmp4
    sed -e '11s/\!//' scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e '12s/\!//' scm_code_ex.tmp5 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

#Modifications according to conditions

if [ "$agg" == 'on' ] && [ "$auto" == 'on' ]; then
#the conditional statemet above needs spacing in
#all the right steps otherwise it will not work

echo ""
echo "agg on ... auto on "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'off' ] && [ "$auto" == 'off' ]; then

#commenting out autoconversion section lines 3-5
    sed -e "3s/^/!/g" scm_code_ex.txt > scm_code_ex.tmp
    sed -e "4s/^/!/" scm_code_ex.tmp > scm_code_ex.tmp2
    sed -e "5s/^/!/" scm_code_ex.tmp2 > scm_code_ex.tmp3

    #the ^ means go to the beginning of the line and add the character !

#commenting out aggregation section lines 10-12
    sed -e "10s/^/!/" scm_code_ex.tmp3 > scm_code_ex.tmp4
    sed -e "11s/^/!/" scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e "12s/^/!/" scm_code_ex.tmp5 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg off ... auto off "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'on' ] && [ "$auto" == 'off' ]; then

#commenting out autoconversion section lines 2-5
    sed -e "3s/^/!/g" scm_code_ex.txt > scm_code_ex.tmp
    sed -e "4s/^/!/" scm_code_ex.tmp > scm_code_ex.tmp2
    sed -e "5s/^/!/" scm_code_ex.tmp2 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg on ... auto off "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'off' ] && [ "$auto" == 'on' ]; then

#commenting out aggregation section lines 10-12

    sed -e "10s/^/!/" scm_code_ex.txt > scm_code_ex.tmp4
    sed -e "11s/^/!/" scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e "12s/^/!/" scm_code_ex.tmp5 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg off ... auto on"
echo "Rebuild model now"
echo ""
fi

I'm very new to programming so I'm wondering how to make this better. For example, can sed be applied to a range of lines in one command? (I could not find something similar). To comment out lines 3-5, I tried sed "3,5{s/^/!/}" scm_code_ex.txt but that didn't work.

Since we're using Bash, we can combine the echo and read by using -p (prompt) argument to read like this:

read -p 'Enable aggregation [on/off] ' agg

I slightly adapted the form of the prompt to be more like common conventions from other interactive programs.

It's a good idea to validate the user input.  If On is entered instead of on, then the user might be surprised to find it's equivalent to off!

unset agg
until [ "$agg" = on ] || [ "$agg" = off ]
do
    read -p 'Enable aggregation [ON/off] ' agg
    agg="${agg:-on}"
done

This also adds a default; that may or may not be appropriate here.

The multiple sed invocations could be combined into one:

sed -e "3s/^/!/" -e "4s/^/!/" -e "5s/^/!/" scm_code_ex.txt

Since we want to perform the same operation on a range of lines, we can use an address range (3,5), and we can write the output file in place of the original (-i):

sed -i -e '3,5s/^/!/' scm_code_ex.txt

(The form you tried didn't work, because you need to separate the { from the enclosed command inside the group - but since there's only a single command, we don't need a group.)

We probably don't want to be doing the in-place editing: it's better to have a pristine (read-only) source file, and always generate the output file starting from there (we can even prefix it with a "DO NOT EDIT" comment).  This makes it more amenable to generation by Make, as well as saving your bacon if you ever make a mistake in the script.

Instead of duplicating the commands for aggregation, one thing we can do is build the appropriate sed command using the options, then run it once.  I would do this using Bash array variables:

replacements=()
[ "$auto" = on ] || replacements+=( -e '3,5s/^/!/' )
[ "$agg" = on ] || replacements+=( -e '10,12s/^/!/' )

sed -e "${replacements[@]}" scm_code_all.txt >scm_code.txt

Now we no longer need all those if/elif/fi blocks, and we no longer double the code size each time we add a new option.

The remaining fragile element in these scripts is the line-number addressing; this makes it hard to see which lines will be enabled or disabled, and it's hard to keep track when we add or remove lines from the source file.  We can add some markers to this file (I'm assuming that ! begins a comment in that language):

code
code

!##BEGIN AUTO
code-auto
code-auto
code-auto
!##END AUTO

code
code

!##BEGIN AGG
code-agg
code-agg
!##END AGG

code
code

We can then use those lines for addressing:

[ "$auto" = on ] || replacements+=( -e '/^!##BEGIN AUTO/,/^!##END AUTO/s/^/!/' )
[ "$agg" = on ] || replacements+=( -e '/^!##BEGIN AGG/,/^!##END AGG/s/^/!/' )

We can reduce the amount of repetition if instead of using variables for auto and agg, we keep a list of features, and loop through them:

features=()
for i in auto agg
do
    while read -p "Enable '$i' feature? [YES/no] " f
    do case "${f,,}" in
           y|yes|on|true|'') features+=($i); break;;
           no|off|false) break;;
       esac
    done
done

replacements=()
for i in ${features[*]}
do
    replacements+=( -e "/^!##BEGIN ${i^^}/,/^!##END ${i^^}/s/^/!/" )
done

And we can combine the two loops into one, thus:

replacements=()
for i in auto agg
do
    while read -p "Enable '$i' feature? [YES/no] " f
    do case "${f,,}" in
           y|yes|on|true|'')
               replacements+=( -e "/^!##BEGIN ${i^^}/,/^!##END ${i^^}/s/^/!/" );
               break;;
           n|no|off|false)
               break;;
       esac
    done
done

With these changes, we no longer need to remove any intermediate temporary files, but I'll note that the wildcard used is rather broad and may catch the user's own temporaries.  Also, the temporaries are in the current working directory, not respecting $TMPDIR.  To fix both of those, I prefer to create a temporary directory with mktemp -d, and trap the EXIT signal to remove all temporaries:

tmpdir=$(mktemp -d)
trap 'rm -r "$tmpdir"' EXIT

I hope this answer gives some food for thought.  If you want a review of your code after considering the above, feel free to ask a new question with your amended code.


Priority Queue in C# I develop a small game using C# and need A* in it. For that, I need a PriorityQueue, which .NET does not have. I wanted to make my own one for practice. Here is it, please comment on performance and usability:

public class PriorityQueue<T> : IEnumerable
{
    List<T> items;
    List<int> priorities;

    public PriorityQueue()
    {
        items = new List<T>();
        priorities = new List<int>();
    }

    public IEnumerator GetEnumerator() { return items.GetEnumerator(); }
    public int Count { get { return items.Count; } }

    /// <returns>Index of new element</returns>
    public int Enqueue(T item, int priority)
    {
        for (int i = 0; i < priorities.Count; i++) //go through all elements...
        {
            if (priorities[i] > priority) //...as long as they have a lower priority.    low priority = low index
            {
                items.Insert(i, item);
                priorities.Insert(i, priority);
                return i;
            }
        }

        items.Add(item);
        priorities.Add(priority);
        return items.Count - 1;
    }

    public T Dequeue()
    {
        T item = items[0];
        priorities.RemoveAt(0);
        items.RemoveAt(0);
        return item;
    }

    public T Peek()
    {
        return items[0];
    }

    public int PeekPriority()
    {
        return priorities[0];
    }
}

I tested it with...

        PriorityQueue<String> pQ = new PriorityQueue<string>();

        for (int i = 0; i < 100; i++)
        {
            int prio = Provider.Rnd.Next(0, 1000);
            pQ.Enqueue(prio.ToString(), prio);
            if (pQ.Count > 0 && Provider.Rnd.Next(0, 2) == 0) pQ.Dequeue();
        }

        while (pQ.Count > 0)
        {
            Console.WriteLine(pQ.Dequeue());
        }

There are other data structures for priority queues. You might consider implementing the queue as a binary heap instead, which gives you a run-time complexity of O(1) for accessing the "largest" (or "smallest", depending on the comparison) element, and O(log n) for insertion and removal (of the largest).


Reader Writer Lock Implementation I'm practicing to write c++11/14 and I implemented a RW lock. Does everything look ok?

class RWLock {
public:
void readLock() {
    std::unique_lock<std::mutex> lock(mutex);
    waitingReaders.wait(lock, [this]{!writer && queuedWriters == 0});
    ++readers;
}

void readUnlock() {
    std::unique_lock<std::mutex> lock(mutex);
    --readers;
    if (readers == 0 && queuedWriters > 0) {
        lock.unlock();
        waitingWriters.notify_one();
    }
}

void writeLock() {
    std::unique_lock<std::mutex> lock(mutex);
    ++queuedWriters;
    waitingWriters.wait(lock, [this]{!writer && readers == 0});
    --queuedWriters;
    writer = true;
}

void writeUnlock() {
    std::unique_lock<std::mutex> lock(mutex);

    if(queuedWriters > 0) {
        lock.unlock();
        waitingWriters.notify_one();
    } else {
        writer = false;
        lock.unlock();
        waitingReaders.notify_all();
    }
}

private:
std::mutex mutex;
std::condition_variable waitingReaders;
std::condition_variable waitingWriters;

int readers = 0;
int queuedWriters = 0;
bool writer = false;
};

In particular:
Do I really need to take a lock in my WriteUnlock() method? It looks like all I'm doing is unlock right away.

And is it recommended to perform a lock.unlock() before I do a notify_one or a notify_all ?

Does performing a condition_variable.wait(lock, lambda) where the lambda is true cause a performance loss? Like potentially in the readLock() method

Do I really need to take a lock in my WriteUnlock() method? It looks like all I'm doing is unlock right away.

YES, you need the lock, because you are accessing queuedWriters and you need to make sure nobody changes it (by calling writeLock) before you notify readers or writers based on the check. You could have seen queuedWriters being zero, therefore notifying readers, but some writer could get sleeping in the meantime and never get woken.

  And is it recommended to perform a lock.unlock() before I do a notify_one or a notify_all?

Unlock before, because the thread being woken would immediatelly try to hold the lock, thus being blocked again (to be woken for second time when you unlock).

  Does performing a condition_variable.wait(lock, lambda) where the lambda is true cause a performance loss?

It is same as while (!pred()) wait(lck);
What else could you do? It checks first, so, would not wait if the condition is true.


Merge two sorted linear graphs I have code for a function to calculator the addition of two sorted List. Each element of List is tuple type (xi, yi) that represents graph individual (x,y) points at coordinate system.

  two input List is sorted by x value and length of A, B list may be different
  [(Ax0, Ay0),..., (Axi, Ayi), (Axi+1, Ayi+1),, ...(Axn, Ayn)][(Bx0, By0),..., (Bxi, Byi), (Bxi+1, Byi+1),, ...(Bxk, Byk)]

example 1:
A:[(1, 1]]=> one point: 1, at x coordinate, 0 at y coordinate
B:[(0, 0]]=> one point: 0, at x coordinate, 0 at y coordinate
=> graph_addition(A, B) == [(0,0), (1,1)]
a line with two point (0,0) and (1,1)

example 2:
A:[(3, 3), (5,5)]=> one line at (3,3), (5,5)
B:[(4, 4), (5, 5), (10, 8)]=> 2 segment line:
ex: graph_addition(A, B) = [(3, 3), (4, 8.0), (5, 10)]
3 segment line with 3 point [(3, 3), (4, 8.0), (5, 10), (10, 8)]
For A when x at 4, y should be 4 based on slope calculation

example 3:
A:[(3,3), (5,5), (6,3), (7,5)]
B:[(0,0), (2,2), (4,3), (5,3), (10,2)]
explanation
when x = 0 at B line, y=0, at A line y = 0 => (0, 0)
when x = 2 at B line, y=2, at A line y = 0 => (0, 0)
when x = 4 at B line, y=3, at A line y = (5-3 / 5-3 * 4) => (4, 7)
when x = 5 at B line, y=3, at A line y = 5 => (5, 8)
when x = 6 at B line, y= (2 - 3)/(10-5) * 6, at A line y = 3 => (6, 0)
when x = 7 at B line, y= (2 - 3)/(10-5) * 7, at A line y = 5 => (7, 1.5)
when x = 10 at B line, y=3, at A line y = (5-3 / 5-3 * 4) => (4, 7)
=> [(0, 0), (2, 2), (3, 5.5), (4, 7.0), (5, 8), (6, 5.8), (7, 7.6), (10, 2)]

Is there a better way for me to not using bruteforce way to calculate x, y values?

def graph_addition(A, B):
     if not A or not B: return A or B
     res = []
     i = j = 0
     while i < len(A) and j < len(B):
         if A[i][0] < B[0][0]:
             x, y = A[i]
             i += 1
         elif B[j][0] < A[0][0]:
             x, y = B[j]
             j += 1
         elif A[i][0] < B[j][0]:
             x = A[i][0]
             y = (B[j][1] - B[j - 1][1]) / (B[j][0] - B[j - 1][0]) * (x - B[j - 1][0]) + B[j - 1][1] + A[i][1]
             i += 1
         elif A[i][0] > B[j][0]:
             x = B[j][0]
             y = (A[i][1] - A[i - 1][1]) / (A[i][0] - A[i - 1][0]) * (x - A[i - 1][0]) + A[i - 1][1] + B[j][1]
             j += 1
         else:
             x = A[i][0]
             y = A[i][1] + B[j][1]
             i += 1
             j += 1
         res.append((x, y))
     if A[i:]: res += A[i:]
     if B[j:]: res += B[j:]
     return res

Use yield instead of building a list. There is no need for an explicit list.
At start of method or input with [] for cleaner None checks.
Move interpolation into a function. This avoid repeated code.
Move all the casework into the interpolation function.

def graph_value(L, i, x):
    if x == L[i][0]: return L[i][1]
    if i == 0: return 0
    m = (L[i][1] - L[i - 1][1]) / (L[i][0] - L[i - 1][0])
    return m * (x - L[i - 1][0]) + L[i - 1][1]

def graph_addition(A, B):
    A = A or []
    B = B or []
    i = j = 0
    while i < len(A) and j < len(B):
        x = min(A[i][0], B[j][0])
        y = graph_value(A, i, x) + graph_value(B, j, x)
        i += (x == A[i][0])
        j += (x == B[j][0])
        yield (x,y)
    yield from A[i:]
    yield from B[j:]

Exercise to the reader: write a version that takes in two arbitrary iterables and performs the same functionality.


CSV parsing program that creates distinct header rows with transaction rows underneath My code reads in the data using DictReader, then creates a header row that contains my composite key (PEOPLE_ID, DON_DATE), and then adds various values that are distinct to each section.  The output looks like this:

-01- PEOPLE_ID, DON_DATE, etc...
-02- dataline
-02- dataline
-01- ...
etc...

I'm looking to possibly simplify or streamline this code, and then could use advice on how to implement robust error-handling throughout.  Here is my program:

#!/usr/bin/python
# pre_process.py
import csv
import sys

def main():
    infile = sys.argv[1]
    outfile = sys.argv[2]
    with open(infile, 'rbU') as in_obj:
        reader, fieldnames = open_reader(in_obj)
        reader = sorted(reader, key=lambda key: (key['PEOPLE_ID'],
                                                 key['DON_DATE']))
        header_list = create_header_list(reader)
        master_dict = mapData(header_list, reader)
        writeData(master_dict, outfile, fieldnames)

def open_reader(file_obj):
    reader = csv.DictReader(file_obj, delimiter=',')
    return reader, reader.fieldnames

def create_header_list(dict_obj):
    p_id_list = []
    for row in dict_obj:
        if (row['PEOPLE_ID'], row['DON_DATE']) not in p_id_list:
            p_id_list.append((row['PEOPLE_ID'], row['DON_DATE']))
    return p_id_list

def mapData(header_list, dict_obj):
    master_dict = {}
    client_section_list = []
    for element in header_list:
        for row in dict_obj:
            if (row['PEOPLE_ID'], row['DON_DATE']) == element:
                client_section_list.append(row)
        element = list(element)
        element_list = [client_section_list[0]['DEDUCT_AMT'],
                    client_section_list[0]['ND_AMT'],
                    client_section_list[0]['DEDUCT_YTD'],
                    client_section_list[0]['NONDEDUCT_YTD']
                    ]
        try:
            element_list.append((float(client_section_list[0]['DEDUCT_YTD']) +
                                 float(client_section_list[0]['NONDEDUCT_YTD'])
                                 ))
        except ValueError:
            pass

        element.extend(element_list)
        element = tuple(element)
        master_dict[element] = client_section_list
        client_section_list = []
    return master_dict

def writeData(in_obj, outfile, in_fieldnames):
    with open(outfile, 'wb') as writer_outfile:
        writer = csv.writer(writer_outfile, delimiter=',')
        dict_writer = csv.DictWriter(writer_outfile,
                                     fieldnames=in_fieldnames,
                                     extrasaction='ignore')

        for k, v in in_obj.iteritems():
            writer_outfile.write(' -01- ')
            writer.writerow(k)
            for i, e in enumerate(v):
                writer_outfile.write(' -02- ')
                dict_writer.writerow(e)

def getReconTotals(infile):
    pass

if __name__ == '__main__':
    main()

Don't reuse names for multiple purposes

Before this line, reader is a DictReader,
after this line it's a list:

    reader = sorted(reader, key=lambda key: (key['PEOPLE_ID'],
                                             key['DON_DATE']))

This can be confusing. It would be better to name the result something else.
And it gets worse: this new reader reader is passed to create_header_list and mapData as parameter named "dict_obj",
which further adds to the confusion.

Simplify set creation

This function essentially creates a set:

def create_header_list(dict_obj):
    p_id_list = []
    for row in dict_obj:
        if (row['PEOPLE_ID'], row['DON_DATE']) not in p_id_list:
            p_id_list.append((row['PEOPLE_ID'], row['DON_DATE']))
    return p_id_list

The not in check is inefficient, because it's an \$O(n)\$ operation.

It would be simpler and more efficient to use a set:

def create_header_list(dict_obj):
    p_id_set = set()
    for row in dict_obj:
        p_id_set.add((row['PEOPLE_ID'], row['DON_DATE']))
    return p_id_set

Or even:

def create_header_list(dict_obj):
    return set([(row['PEOPLE_ID'], row['DON_DATE']) for row in dict_obj])

If the ordering of the elements is important,
then instead of a set, you can use an OrderedDict,
as suggested by this post.

Running Python scripts

Not all systems have Python at /use/bin/python. The recommended shebang for Python scripts:

#!/usr/bin/env python

Follow PEP8

PEP8 is the coding style guide for Python.
Among other things,
it recommends using snake_case for variable and function names.
Several functions violate that.

Even if you disagree with a specific naming convention,
it's a universal violation of good naming practices to mix two kinds of naming styles in the same program, such as create_header_list and mapData.


Calculation with Jquery (function ($) {

    "use strict";

    $(document).ready(function () {

        $('.bookinginput').on('change', function(){

            var adults = $('#t_adults_qty').val(),
                    child =  $('#t_children_qty').val(),
                    pickup = $('#pick_up_list').val(),
                    priceBase = priceBooking.adult * adults,
                    priceChild = priceBooking.child * child;

            var price = priceBase + priceChild

            $('#price_total').val(price);
        })

    });
}(jQuery));

I will explain my code a bit, basically it is a small mathematical calculation based on the value of the inputs.

I have a json with the variable priceBooking, which has two values (Adult and Child), and I have several inputs with the numbers of the (adults and children).

I simply calculate the number of (Adults for the Adult price) the same with the children.

and the result I put it in the value of another hidden input (#price_total)

I would like to simplify this code or improve it. I want to be able to work with variables globally and not have to duplicate code.

Any suggestions?

pickup is defined, but never used.

You should terminate your statements with semicolons consistently.


Different date formats I'm using Java 7.

private Long difference;
public Long timeDifference(String weboutput) {
    try {
        Calendar calendar = GregorianCalendar.getInstance();
        Calendar today = new GregorianCalendar();
        Date inputTime;
        if (weboutput.length() <= 11) { // for data fetched for current date.
            DateFormat formatter = new SimpleDateFormat("hh:mm:ss a", Locale.US);
            inputTime = formatter.parse(weboutput);
            calendar.setTime(inputTime);
            int hour = calendar.get(Calendar.HOUR);
            int minute = calendar.get(Calendar.MINUTE);
            int second = calendar.get(Calendar.SECOND);
            today.setTime(new Date());
            today.set(Calendar.HOUR, hour);
            today.set(Calendar.MINUTE, minute);
            today.set(Calendar.SECOND, second);
        } else {
            if (weboutput.length() <= 15) { // for data for earlier date in same year or month.
                DateFormat formatter = new SimpleDateFormat("MMM dd hh:mm a", Locale.US);
                inputTime = formatter.parse(weboutput);
                calendar.setTime(inputTime);
                int hour = calendar.get(Calendar.HOUR);
                int minute = calendar.get(Calendar.MINUTE);
                int month = calendar.get(Calendar.MONTH);
                int date = calendar.get(Calendar.DATE);
                today.setTime(new Date());
                today.set(Calendar.HOUR, hour);
                today.set(Calendar.MINUTE, minute);
                today.set(Calendar.MONTH, month);
                today.set(Calendar.DATE, date);
            } else { // for data with different year.
                DateFormat formatter = new SimpleDateFormat("MMM dd, yyyy hh:mm:ss a", Locale.US);
                inputTime = formatter.parse(weboutput);
                calendar.setTime(inputTime);
                int hour = calendar.get(Calendar.HOUR);
                int minute = calendar.get(Calendar.MINUTE);
                int second = calendar.get(Calendar.SECOND);
                int month = calendar.get(Calendar.MONTH);
                int date = calendar.get(Calendar.DATE);
                int year = calendar.get(Calendar.YEAR);
                today.setTime(new Date());
                today.set(Calendar.HOUR, hour);
                today.set(Calendar.MINUTE, minute);
                today.set(Calendar.SECOND, second);
                today.set(Calendar.YEAR, year);
                today.set(Calendar.MONTH, month);
                today.set(Calendar.DATE, date);
            }
        }
        Date retrivedDate = today.getTime();
        Calendar cal = Calendar.getInstance();
        Date currentDate = cal.getTime();
        difference = currentDate.getTime() - retrivedDate.getTime();
        System.out.println(retrivedDate);
        System.out.println(currentDate);
        System.out.println(difference);

    } catch (ParseException e) {
        e.printStackTrace();
    }
    return difference;
}

public boolean alarmValue(Long alarmTime) {
    if (alarmTime <= 1800000) // change this value for Alarm duration, currently 30 min = 30* 60 s = 1800 * 1000 ms = 1800000 ms.
        return false;
    else
        return true;
}

Do you see what I see? Because I see copy-pasta: Yummy :)

            int hour = calendar.get(Calendar.HOUR);
            int minute = calendar.get(Calendar.MINUTE);
            int second = calendar.get(Calendar.SECOND);
            int month = calendar.get(Calendar.MONTH);
            int date = calendar.get(Calendar.DATE);
            int year = calendar.get(Calendar.YEAR);
            today.setTime(new Date());
            today.set(Calendar.HOUR, hour);
            today.set(Calendar.MINUTE, minute);
            today.set(Calendar.SECOND, second);
            today.set(Calendar.YEAR, year);
            today.set(Calendar.MONTH, month);
            today.set(Calendar.DATE, date);

This (or subsections thereof) are copy-pasted for each block in your method.

This screams for extraction into a method:

private static void copyCalendarFields(Calendar source, Calendar destination,
  int... fields) {
    for (int field : fields) {
       destination.set(field, source.get(field));
    }
}

And suddenly things become significantly smaller:

        int hour = calendar.get(Calendar.HOUR);
        int minute = calendar.get(Calendar.MINUTE);
        int second = calendar.get(Calendar.SECOND);
        today.setTime(new Date());
        today.set(Calendar.HOUR, hour);
        today.set(Calendar.MINUTE, minute);
        today.set(Calendar.SECOND, second);

is just:

copyCalendarFields(calendar, today,
    Calendar.HOUR, Calendar.MINUTE, Calendar.SECOND);

and suddenly these long blocks of code are gone and the whole method is a lot more digestable