Unit-testing an RSS feed parser I'm working on a class to parse RSS feeds using SyndicationFeed. (related to Downloading data using HttpClient).  I'm trying to write this so it can be unit-tested, but there's an awful lot of boring setup boilerplate before I get to the important stuff.

My data source sometimes sends invalid RSS XML according to XmlReader (in the pattern of <description>This article ... <mark>keyword</mark> ...</description>), so I need to sanitize the input (for political reasons, I can't just say "this won't work until they fix their end").

I started with the following method, which does a few too many things and isn't very testable.  My important logic starts after the SyndicationFeed.Load() call, but any tests require that all the XML handling works correctly.

public virtual FeedSummary ParseResponse(string data)
{
    var response = new FeedSummary();

    var doc = new XmlDocument();
    doc.LoadXml(data);

    var builder = new StringBuilder();
    var xmlSettings = new XmlWriterSettings();
    xmlSettings.OmitXmlDeclaration = true;
    xmlSettings.ConformanceLevel = ConformanceLevel.Fragment;
    var htmlWriter = XmlWriter.Create(builder, xmlSettings);

    var transformationDocument = new XmlDocument();
    transformationDocument.LoadXml(this.XsltSource);
    //The xslt source can be passed in via the class constructor so
    //I can test this using the identity transform
    var compiledTransform = new XslCompiledTransform(true);
    compiledTransform.Load(transformationDocument);
    compiledTransform.Transform(doc, htmlWriter);

    var reader = XmlReader.Create(new StringReader(builder.ToString()));
    var feed = SyndicationFeed.Load(reader);
    reader.Close();

    var totalResults = feed.ElementExtensions.Single(x => x.OuterName == "totalResults");
    response.TotalResults = Int32.Parse(totalResults.ToString());
    foreach (var item in feed.Items)
    {
        var hits = new List<SearchHit>();
        var searchItem = new SearchHit()
        {
            Title = item.Title.Text,
            ItemUrl = item.Links[0].Uri,
            Description = item.Summary.Text
        };
        hits.Add(searchItem);
        response.TopHits = hits;
    }

    return response;
}

I did try breaking it up into multiple methods, coming together in this one:

public FeedSummary ParseResponse(string rssFeed)
{
    var doc = LoadXml(rssFeed);
    var cleanData = SanitizeXml(doc);
    var feed = CreateFeed(cleanData);
    return ParseFeed(feed);
}

This is better than the first one in that each step can be tested independently, but my top-level method still has the same testability problems.  I'd appreciate any thoughts on improving code organization, especially with respect to improving testability.

By breaking the code into separate methods, you've made it easier to read, which is good. You've not made it easier to test though (unless you expose those methods to the test framework, which then creates a leaky abstraction and brittle tests). Therefore all you should be testing with your revised code is ParseResponse itself. ParseResponse has lots of side-effects though, so is hard to test.

The solution is to decouple those side-effect causing parts of the code and to inject them:

public FeedSummary ParseResponse(string rssFeed,
                                 IXMLLoader loader,
                                 IXMLSanitizer sanitizer,
                                 IFeedCreator feedCreator)
{
    var doc = loader.LoadXml(rssFeed);
    var cleanData = sanitizer.SanitizeXml(doc);
    var feed = feedCreator.CreateFeed(cleanData);
    return ParseFeed(feed);
}

The you can create implementations of IXMLLoader etc that can be individually tested with integration tests and provide mocks for testing ParseResponse itself.


simple ThreadPool implementations I would like to ask you for a code review of my c++11 Thread Pool implementation. Your constructive criticism is welcome! Could you give me some ideas how to extend it?

The main idea is to synchronise access to tasks with condition variable.

ThreadPoolExecutor.cpp

#include "ThreadPoolExecutor.h"
#include <algorithm>
#include <iostream>

namespace ThreadPool
{
    ThreadPoolExecutor::ThreadPoolExecutor(int maxWorkers)
    {
        this->maxWorkers = maxWorkers;
    }

    ThreadPoolExecutor::~ThreadPoolExecutor()
    {
        this->poolManager.join();
    }

    void ThreadPoolExecutor::ScheduleTask(std::function<void()> task)
    {
        std::unique_lock<std::mutex> lock(taskQueueMutex);
        bool wasEmpty = this->taskQueue.empty();
        this->taskQueue.push(task);
        if (wasEmpty)
        {
            this->notEmptyTaskQueue.notify_one();
        }

        lock.unlock();
    }

    void ThreadPoolExecutor::Run()
    {
        this->poolManager = std::thread(&ThreadPoolExecutor::ManagePool, this);
    }

    void ThreadPoolExecutor::ManagePool()
    {
        for (int i = 0; i < this->maxWorkers; i++)
        {
            this->workers.push_back(std::thread(&ThreadPoolExecutor::Worker, this));
        }

        for (auto &thread : this->workers)
        {
            thread.join();
        }
    }

    void ThreadPoolExecutor::Worker()
    {
#ifdef _DEBUG
        std::cout << "Hello from thread: " << std::this_thread::get_id() << std::endl;
#endif
        while (1)
        {
            std::unique_lock<std::mutex> lock(taskQueueMutex);
            if (this->taskQueue.empty())
            {
                this->notEmptyTaskQueue.wait(lock);
            }

            auto task = this->taskQueue.front();
            this->taskQueue.pop();
            lock.unlock();
            task();
        }
    }
}

ThreadPoolExecutor.h

#ifndef THREADPOOLEXECUTOR_H_
#define THREADPOOLEXECUTOR_H_

#include <functional>
#include <queue>
#include <thread>
#include <memory>
#include "Task.h"
#include <condition_variable>

namespace ThreadPool
{
    class ThreadPoolExecutor
    {
    public:
        ThreadPoolExecutor(int maxWorkers);
        ~ThreadPoolExecutor();
        void ScheduleTask(std::function<void()> task);
        void Run();

    private:
        int maxWorkers;
        std::vector<std::thread> workers;
        std::thread poolManager;
        std::queue<std::function<void()>> taskQueue;
        std::condition_variable notEmptyTaskQueue;
        void ManagePool();
        std::mutex taskQueueMutex;
        void Worker();
    };
}
#endif

And finally usage.

Program.cs

#include "ThreadPoolExecutor.h"
#include "ExampleTask.h"
#include <iostream>

using namespace ThreadPool;

int main()
{
    ThreadPoolExecutor executor = ThreadPoolExecutor(4);

    for (int i = 0; i < 10; i++)
    {
        int random = std::rand() % 6;
        std::chrono::seconds timeout(random);
        executor.ScheduleTask([i, timeout](){
            std::cout << "no " << i << std::endl;
            std::this_thread::sleep_for(timeout);
        });
    }

    executor.Run();

    ExampleTask task;
    executor.ScheduleTask(task);
    std::this_thread::sleep_for(std::chrono::seconds(15));

    for (int i = 0; i < 10; i++)
    {
        int random = std::rand() % 6;
        std::chrono::seconds timeout(random);
        executor.ScheduleTask([i, timeout](){
            std::cout << "no " << i << std::endl;
            std::this_thread::sleep_for(timeout);
        });
    }

    return 0;
}

ThreadManager.h

What is Task.h? Why are you including it? I see no names that aren't covered by the standard headers you include.

There's no apparent order to your private members. You mix methods and data members; it looks like the members are added in the order they were written, as opposed to some logical order. I recommend putting your methods before your data members (my recommendation matches the Google C++ Style Guide), and determining a logical order within each set.

In general, the header is the place to document your interface. What are the semantics of ScheduleTask? Can it be called from multiple threads without mutual exclusion? Will Run return? Does ThreadPoolExecutor reuse threads after there work is complete? If so, will ThreadPoolExecutor ever destroy threads after creating them? Does the destructor block?

Worker is an odd name for a method. It is the usual practice that function names should be some sort of verb phrase; Worker is a noun, and it doesn't give a lot of insight into what it does.

maxWorkers should probably be declared const; it shouldn't change after construction.

Comments on the private data members should clearly indicate which members are guarded by the mutex.

ThreadManager.cpp

Constructors should use initializers wherever possible:

ThreadPoolExecutor::ThreadPoolExecutor(int maxWorkers) : maxWorkers(maxWorkers) {}

This will allow the member to be declared const.

Your destructor throws if Run has never been called: A default constructed thread is not joinable. That's not the biggest problem: Your destructor never returns if maxWorkers > 0 because none of your calls to join ever complete.

It is distracting (and unidiomatic!) to see unnecessary this-> warts prepended to each member access. If you must have a visual indicator that you are accessing members rather than local variables, adopt a naming convention for your data members (common ones I've seen: maxWorkers_, m_maxWorkers).

In ScheduleTask, it is unnecessary to call lock.unlock();. std::unique_lock unlocks its owned mutex in its destructor; when I see explicit unlocking, I expect that it's because of complicated locking semantics (e.g. conditional unlocking, or unlocking before lock goes out of scope, as in Worker) and look around for the complication; there's no such complication here.

maxWorkers appears to be a misnomer as you never have fewer workers. Maybe numWorkers?


Substring erase with certain pattern For following input string:

"VISITOR_INFO1_LIVE=USv90B-7CzI; LOGIN_INFO=e486e37a395be3f0e3b3237d090a6829c1oAAAB7IjQiOiAiREVMRUdBVEVEIiwgIjciOiAwLCAiMSI6IDEsICIzIjogMjAxMTk0MTMwNiwgIjgiOiA2MDgwMTg0NTEzNjQsICIxMCI6IDIzOTYyMTEyODczNH0=; PREF=f5=30;HSID=AHuJQBOVR0lQoRt_3; APISID=QaParXGsQcEPCzKg/A1smCfYrfMjxvfEPT; YSC=Vm3Amq5loFM";

I want to remove all the patterns that contains *SID (HSID,APISID here) upto ';'. I also want to remove substring "LOGIN_INFO= ....;"

So, output string should be:

"VISITOR_INFO1_LIVE=L80EDuHCEF8; PREF=f5=30";`

 const char *str ="VISITOR_INFO1_LIVE=USv90B-7CzI; LOGIN_INFO=e486e37a395be3f0e3b3237d090a6829c1oAAAB7IjQiOiAiREVMRUdBVEVEIiwgIjciOiAwLCAiMSI6IDEsICIzIjogMjAxMTk0MTMwNiwgIjgiOiA2MDgwMTg0NTEzNjQsICIxMCI6IDIzOTYyMTEyODczNH0=; PREF=f5=30;HSID=AHuJQBOVR0lQoRt_3; APISID=QaParXGsQcEPCzKg/A1smCfYrfMjxvfEPT; YSC=Vm3Amq5loFM";
 char *Cookie = NULL;
 cout << "original string is:\n" << str << "\n";
 int len = strlen(str)+1;
 cout << "length of original string is : " << len << "\n";
 Cookie = new char[strlen(str)];
 strncpy(Cookie,str,len);

 char *p1 = strstr(Cookie,"LOGIN_INFO");
 char *p2 = NULL;
 if(p1){
      p2 = strstr(p1,";")+1;
      while(*p2 == ' ') p2++;
 }
 if(p1 && p2)
      memmove(p1,p2,strlen(p2)+1);

 char *ID = strstr(Cookie,"SID");
 while( ID != NULL){
      char *start_pos = NULL, *end_pos = NULL;
      while((*ID != ';') && (*ID != Cookie[0]) && (*ID != ' ')){
           --ID;
      }
      if(*ID == Cookie[0]) start_pos = ID;
      else start_pos = ID+1;
      end_pos = strstr(start_pos,";")+1;
      while(*end_pos == ' ')
           end_pos++;
      memmove(start_pos,end_pos,strlen(end_pos)+1);
    //  }
      /*else
           std::cout << "does not find substr " << "\n";*/

    //  cout << "modified string is :" << Cookie << "\n";
      ID = strstr(Cookie,"SID");
 }
      //cout << "final modified string is : " << Cookie << "\n";
 char *Cookie_modified = NULL;
 const char *pch = strstr(Cookie,"PREF");
 if(pch != NULL){
      const char *append = "&f2=8000000";
      int len = strlen(Cookie) + strlen(append) + 1;
      Cookie_modified = new char[len];
      strncpy(Cookie_modified,Cookie,len);
      Cookie_modified[len-1] = '\0';
      char *p = strstr(Cookie_modified,"PREF");
      strncpy(p+(strlen(p)),append,strlen(append));
      cout << "modified Cookie is : " << Cookie_modified << "\n";
  //    cout << "length of modified cookie is : " << strlen(Cookie_modified) << "\n";
 }

 else{
      cout << "do not find reference: " << "\n";
      const char *append = ";PERF=f2=8000000";
      int len = strlen(Cookie) + strlen(append) + 1;
      Cookie_modified = new char[len];
      Cookie_modified[len-1] = '\0';
      strcat(Cookie_modified,Cookie);
      strcat(Cookie_modified,append);
      cout << "case 2: modified Cookie is: " << Cookie_modified << "\n";

 }
 delete[] Cookie;
 delete[] Cookie_modified;
 return 0;

Comment on Question
Can tell if your code matches the problem statement.
Because I can't understand the problem statement.
Input

"VISITOR_INFO1_LIVE=USv90B-7CzI; LOGIN_INFO=e486e37a395be3f0e3b3237d090a6829c1oAAAB7IjQiOiAiREVMRUdBVEVEIiwgIjciOiAwLCAiMSI6IDEsICIzIjogMjAxMTk0MTMwNiwgIjgiOiA2MDgwMTg0NTEzNjQsICIxMCI6IDIzOTYyMTEyODczNH0=; PREF=f5=30;HSID=AHuJQBOVR0lQoRt_3; APISID=QaParXGsQcEPCzKg/A1smCfYrfMjxvfEPT; YSC=Vm3Amq5loFM";

Output

"VISITOR_INFO1_LIVE=L80EDuHCEF8; PREF=f5=30";

Questions

Is the first and last quote and the last semi colon part of the input or just your formatting?
*SID (HSID,APISID here) upto ';'
I see none of these things in the output. there is no SID no HSID no APISID
Whey is  'VISITOR_INFO1_LIVE' in the output?
It is not specified as something you want in the output.
Why is PREF in the output.
This PREF=f5=30 seems like an illegal parameter. is = allowed in the parameter.

Review of code
OK. I assume there is a function wrapper here that defines str!! What is the type of str? I am going to assume char const* as you seem to be coding using C (rather than real C++). Types are super important in C++ please make sure we know all the types of all identifiers.
Why split up the declaration from the initialization?
 char *Cookie = NULL;
 ......
 Cookie = new char[strlen(str)];
 strncpy(Cookie,str,len);
 ......
 delete[] Cookie;

This just gives you the opportunity to misuse Cookie in the code. If you declare and initialize in the same statement then no misuse.
Also C++ code should never contain a delete (especially when you are learning). It should have very few calls (if any calls) to new. Dynamic memory management should be controled via an object. This is usually a smart pointer or a container depending on your usage.
Thirdly why use the C-String API. C++ has its own string that handles memory management for you and does a better job. Currently your code is not exception safe. An exception thrown in your code will result in leaked memory.
Thirdly you actually did it wrong and did not allocate enough space (you forgot to allocate space for the '\0' character that terminates a C-String). I know you used the strncpy to compensate for this. But all the other string functions are expecting it so something is likely to go wrong.
Forgot to check the result of a string function.
      p2 = strstr(p1,";")+1;

If strstr() returns NULL then we add 1 to it and now it is no longer NULL and your next test will always work. Even if it is not supposed too.
Are you sure the second test id correct?
      while((*ID != ';') && (*ID != Cookie[0]) && (*ID != ' ')){
           --ID;
      }

I think you are testing here *ID != Cookie[0] to make sure that you don't fall off the front of the string. But you are checking the content of the characters not the position of the pointers. A better test is (ID > Cookie)


Capitalizing the first letter of every word in a string (with arbitrary spacing) I solved the following question

You are asked to ensure that the first and last names of people begin
with a capital letter in their passports. For example, alison heck
should be capitalised correctly as Alison Heck.
Given a full name, your task is to capitalize the name appropriately.
Input Format
A single line of input containing the full name, S.
Constraints

0 < len(S) < 1000
The string consists of alphanumeric characters and spaces.

Note: in a word only the first character is capitalized.
Example 12abc when capitalized remains 12abc.
Output Format:
Print the capitalized string, S.

Here's my code:
#!/bin/python

import math
import os
import random
import re
import sys

# Complete the solve function below.
def solve(s):
    ls = s.split(" ")
    new_word = ""
    new_name = ""
    for word in ls:
        if len(word) > 1:
            new_word = new_word + word[0].title() + word[1:] + " "
        else:
            new_word = new_word + word.title() + " "
    new_name = new_name.join(new_word)
    return new_name

if __name__ == '__main__':
    fptr = open(os.environ['OUTPUT_PATH'], 'w')

    s = raw_input()

    result = solve(s)

    fptr.write(result + '\n')

    fptr.close()

I think I used a very long approach and would appreciate more efficient techniques to do the same.
The primary problem which I faced is handling arbitrary spaces and hence I went with this approach but the code smells bad. One things I can think of is reducing variables but that wouldn't be very helpful and make it less readable I think
EDIT : The question is  this question from HackerRank however the solution to this problem on the website does not include test cases where capitalize() and title() fail. The failing of these and more methods have also been discussed here

remove the unused imports

Your code logic is OK but the execution is not good. Try not to overcomplicate it. Take a piece of paper and write the procedure of how a human like YOU would do it, just with a pencil and paper.
read the names individually... make first character capital if it isn't a digit

Now that you have  basic design, become more specific, or in Python terms

read the names individually for word in string.split()
make the first character capital: string.title()
if it isn't a digit : if not string[0].isdigit()

The primary problem which I faced is handling arbitrary spaces

string.split() will return the same thing, let it be 1 space or 1000 spaces. It does not matter
Now you have exactly what you need, it is just a matter of putting it together.
for every word in words, capitalize if the first character isn't a digit else do nothing

    return " ".join(word.title() if not word[0].isdigit() else word for word in words.split())

Furthermore, using capitalize() will avoid the extra check
    return " ".join(word.capitalize() for word in words.split(' '))

EDIT:
You have to use .split(' ') and NOT .split() since .split() removes all the whitespaces.

As you mentioned, title() and capitalize() fail for scenarios where you pass something like
ALLISON heck

Output
Allison Heck

In that case, you need to have extra checks. The best thing to do here is to create another function that specifically capitalizes the first letter
Here is what I thought of
def cap_first(word):
    return word[:1].upper() + word[1:]

the solve function remains the same
def solve(words):
    return ' '.join(cap_first(word) for word in words.split(' ')

Benchmarks
the latter code is surely more readable and compact, but what is its performance compared to the previous solution?
I will measure the execution time in the following manner using the time module
for iterations in (10 ** 5,11 ** 5, 10 ** 6, 11 ** 6):
    print(f"\n{iterations} iteartions\n")

    start = time.time()
    for _ in range(iterations): solvenew(names)
    print(f"Time taken for new function: {time.time() - start:.3f} s")

    start = time.time()
    for _ in range(iterations): solveoriginal(names)
    print(f"Time taken for original function: {time.time() - start:.3f} s")

Here are the results
#                      Time taken
#
#     iterations  |  original    |   new
#   --------------------------------------
#       10 ** 6   |   2.553 s    |  2.106 s
#   --------------------------------------
#       11 ** 6   |   6.203 s    |  5.542 s
#   --------------------------------------
#       10 ** 7   |   32.412 s   |  24.774 s

Feel free to try it yourself


Minimum Number of swaps required to group all ones together in array of 0s and 1s Problem Statement:

Given a binary array data, return the minimum number of swaps required to group all 1’s present in the array together in any place in the array.

Example 1:

Input: [1,0,1,0,1]

Output: 1

Explanation:

There are 3 ways to group all 1's together:

[1,1,1,0,0] using 1 swap.

[0,1,1,1,0] using 2 swaps.

[0,0,1,1,1] using 1 swap.

The minimum is 1.

Example 2:

Input: [0,0,0,1,0]

Output: 0

Explanation:

Since there is only one 1 in the array, no swaps needed.

Example 3:

Input: [1,0,1,0,1,0,0,1,1,0,1]

Output: 3

Explanation:

One possible solution that uses 3 swaps is [0,0,0,0,0,1,1,1,1,1,1].

This is my solution. I used sliding window.

First I counted total number of ones in the given array.

To group the 1s in minimum number of swaps we have to group them up in a cluster where their density is already more.

So I made an array which keeps a count of 1s encountered till that element

Then I created a window of the size of total 1s in that array and moved that window till the end to find which sub array had maximum number of 1s, because this is the area where we will insert other 1s

The code is as follows

int minSwaps(vector<int>& data) {
        // Size of given array
        int size = data.size();

        // Total ones in given array
        int totalOnes = 0;

        // Counting total ones in array
        for(int i = 0 ; i < size;++i){
            if(data[i]==1) totalOnes++;
        }

        if(totalOnes==size) return 0;
        if(totalOnes==0) return 0;

        // Using a winow of the size of total ones
        int window = totalOnes;

        // maxOnes will store the value of maximum ones in any sliding window of size 'window'
        int maxOnes = INT_MIN;

        // This array will store total number of ones till index i
        vector<int> onesTillNow(size,0);

        if(data[0]==1)
            onesTillNow[0]=1;
        else
            onesTillNow[0]=0;

        for(int i = 1; i < size;++i){
            if(data[i]==1)
                onesTillNow[i] = onesTillNow[i-1] + 1;
            else
                onesTillNow[i] = onesTillNow[i-1];
        }

        for(int i = window - 1; i < size ; ++i){
            if(i == window - 1 )
                totalOnes = onesTillNow[i];
            else
                totalOnes = onesTillNow[i] - onesTillNow[i-window];

            if(totalOnes > maxOnes) maxOnes = totalOnes;
        }
        return window - maxOnes;
    }

Can you suggest any improvements to this or a better way to do it?

The algorithm is sound, and it's O(N). There are only two things I see you can improve.

Make use of the standard library

The C++ library comes with a lot of functions, including ones that can take care of summing elements of a vector for you. So to find the number of ones in the input vector, you can just write:

#include <algorithm>
...
int window = std::accumulate(std::begin(data), std::end(data), 0);

You don't need onesTillNow

When moving your window over the input array, you don't need to store all the number of ones for every window position. You just want to know the maximum number of ones in a window. If you just have a single variable int onesInWindow, and initialize it with the sum of ones in the first window:

int onesInWindow = std::accumulate(std::begin(data), std::begin(data) + window, 0);
int maxOnes = onesInWindow;

Then you can iterate over all the other window positions like this:

for(int i = window; i < size; i++) {
    onesInWindow -= data[i - window];
    onesInWindow += data[i];
    maxOnes = std::max(maxOnes, onesInWindow);
}


Syncing e-mails for multiple accounts I wrote an email syncing program in my ASP.NET MVC website that syncs e-mails from multiple e-mail accounts to my database.

Each e-mail account has to be independent from other accounts and they have to run asynchronously.

Are there are any issues with my code such as efficiency, bugs or can there be made improvements?

   public void Start()
    {
        new Thread(StartSyncing).Start();
    }

   private void StartSyncing()
    {
        foreach (var emailAccount in _emailAccountRepository.GetList())
        {
            // Create a local copy
            EmailAccount copyEmailAccount = emailAccount;

            // Start a new thread for each email account
            new Thread((x => SyncAccount(copyEmailAccount))).Start();
        }
    }

   private void SyncAccount(EmailAccount emailAccount)
    {
        // Sync forever
        while (true)
        {
            // Set the next syncing date
            _nextSyncingDate = DateTime.Now.AddMinutes(5);

            // Create a fresh start for the database context
            InitRepositories();

            // Connect to the mail server
            using (var emailProvider = new ReceiveEmailProvider(emailAccount.Email,
                emailAccount.Password, emailAccount.Host, emailAccount.Port, emailAccount.Secure))
            {
                // For each email
                for (int emailCounter = emailProvider.GetAmountEmails(); emailCounter > 0; emailCounter--)
                {
                    // As long the email is new
                    while (IsNewEmail(emailCounter))
                    {
                        // Get email
                        var email = emailProvider.GetEmail(emailCounter, emailAccount.Id);

                        // Check if the email isn't spam
                        if (!IsSpam(GetEmailDomain(email.FromEmail), emailAccount.User.UserId))
                        {
                            // Add the new email
                            var dbEmail = AddNewEmail(email, emailAccount.Id, emailCounter);

                            // Add email to category
                            AddEmailToCategory(emailAccount.User.UserId, dbEmail);
                        }
                    }
                }
            }
            // Set end starting sync date
            _endSyncingDate = DateTime.Now;

            // Get the difference in time between the end and starting date
            var timeDifference = _nextSyncingDate - _endSyncingDate;

            // Sleep if we haven't wait for 5 minutes
            if (_nextSyncingDate > _endSyncingDate)
            {
                // Sleep till the 5 minutes have passed
                Thread.Sleep((int)timeDifference.TotalMilliseconds);
            }
        }
    }

I added the methods that seem relevant to me. If you require more code, let me know!

I feel like some of the code you haven't shown has a few leaky abstractions. For example:

using (var emailProvider = new ReceiveEmailProvider(emailAccount.Email,
            emailAccount.Password, emailAccount.Host, emailAccount.Port, emailAccount.Secure))

Every argument you send to RecieveEmailProvider comes from emailAccount. I would add an overload to it's constructor so that you could simply pass in an emailAccount. I think it greatly simplifies the code.

using (var emailProvider = new RecieveEmailProvider(emailAccount))

There's something similar happening here.

var dbEmail = AddNewEmail(email, emailAccount.Id, emailCounter);

// Add email to category
AddEmailToCategory(emailAccount.User.UserId,dbEmail);

Why does the client code go through all the trouble of spelling this out? AddEmailToCategory should know how to get the info it needs from an emailAccount so the dev doesn't have to.


Binary search use in simple guessing game I'm making a simple guessing game in C. Basically what I need is some help double-checking my work, more specifically a verification that the my binary search in my case statements look okay. The goal is to have an initial guess of 50, then work off that. If "L" is selected, the search goes from 51-100, if "H", it would be 1-49, blah-blah - the usual stuff scaling down to the correct guess.

Is this correct?

I'm also getting a C99 function implementation error using the actual letters as case labels -- just to explain why they're the ASCII equivalents.

#include <stdio.h>
#include <ctype.h>

int main(int argc, const char * argv[])
{
int low;
int high;
int guess;
int response;

 printf("Pick an integer from 1 to 100 and I will try to guess it.\n");
 printf("If I guess too low, respond with a L.\n");
 printf("If I guess too high, respond with a H.\n");
 printf("If I guess correctly, respond with a Y.\n");

high = 100;
low = 1;

while(response != 'Y')
{
guess = (high + low) / 2;

printf("Okay, let's start. Is it %d?\n", &guess);
printf("Please respond with L, H, or Y.\n");
scanf("%d", &response);
response = toupper(response);

switch(response)
{
    case 72: high = guess - 1;
            break;
    case 76: low = guess + 1;
            break;
    case 89: printf("Yes! I knew I could do it.\n");
          break;
    default:  printf("I didn't get that; you gave an invalid response.\n");
              printf("Please try again.\n");
}

}
return 0;
}

I'm thoroughly puzzled by your use of the response variable.

The condition for the while-loop is response != 'Y'.  The first time through, response is an uninitialized variable, containing any imaginable int value.  So, the behaviour of your program is undefined.

response is an int.  Why are you comparing it to 'Y'?  That makes 89 a special number.

If we make it inside the loop, you ask the user to enter a character 'L', 'H', or 'Y', but actually read an integer.  If you actually enter one of those three characters, scanf() will fail to read an integer; your program will consider the input to be invalid, and it will loop forever, since scanf() will immediately fail again, etc.  There are only two ways to terminate your program: CtrlC or by typing 89Enter at the prompt.

Then you call toupper() on the integer; the toupper() function only makes sense for translating a letter to its uppercase counterpart.  (The toupper() function takes an int instead of a char only because it is meant to support EOF, or -1, as a value.)

Then, in the switch block, you have magic numbers 72, 76, and 89.  Magic numbers should not exist in your code.

Your first printf() call is also wrong.  It should be

printf("Okay, let's start. Is it %d?\n", guess);

since &guess would print the address of the guess variable instead of its contents.  Your compiler should have warned you about that mistake.  You should have heeded the warning, as well as tested your code.


Bubble Sort Implementation | C-ish and Modern C++ For practice purpose, I wrote bubble sort in C++ two styles: C-ish and modern C++ compliant. I would like to have your comments on any point on these implementations.

Sorting.h

#ifndef SORTING_ALGORITHMS_H
#define SORTING_ALGORITHMS_H

#include <utility>
#include <vector>
//#include <iterator>
#include <algorithm>

namespace etpc
{
    template <class T>
    void sortBubble(T* pArrHead, std::size_t sArrSize)
    {
        for(std::size_t i=0; i<sArrSize-1; i++)
        {
            for(std::size_t j=0; j<sArrSize-i-1; j++)
            {
                if(pArrHead[j]>pArrHead[j+1])
                    std::swap(pArrHead[j], pArrHead[j+1]);
            }
        }
    }

    // May 2, 2020
    // Based on the following link
    // https://rosettacode.org/wiki/Sorting_algorithms/Bubble_sort#C.2B.2B
    // typename vs class
    // https://stackoverflow.com/questions/2023977/difference-of-keywords-typename-and-class-in-templates
    template <class T>
    void sortBubble(std::vector<T>& vArrHead)
    {
        typename std::vector<T>::iterator begin = std::begin(vArrHead);
        typename std::vector<T>::iterator end = std::end(vArrHead);
        while (begin != end--) {
            for (auto it = begin; it != end; ++it) {
                if (*(it + 1) < *it) {
                    std::iter_swap(it, it + 1);
                }
            }
        }
    }
}
#endif // SORTING_ALGORITHMS_H

Main.cpp

#include "Sorting/include/Sorting.h"

#include <iostream>
#include <chrono>
#include <iterator>

int main()
{
    std::chrono::steady_clock::time_point begin;
    std::chrono::steady_clock::time_point end;
    std::ostream_iterator<int> out_it (std::cout,", ");

    static const int i32Size = 50;
    int arr[i32Size] = {7804, 50398, 14945, 1814, 51383, 63156, 8432, 58103, 28175, 4339, 8361, 37158, 1529, 43066, 62052, 9591, 13168, 332, 55913, 2418, 48066, 46504, 52922, 39523, 36653, 30402, 9373, 56202, 50539, 41187, 42606, 32278, 63902, 41668, 7505, 46534, 25846, 49739, 63411, 45933, 15042, 6544, 35718, 17035, 34647, 15212, 52690, 64299, 61535, 45071};
    begin = std::chrono::steady_clock::now();
    etpc::sortBubble<int>(arr, i32Size);
    end = std::chrono::steady_clock::now();
    std::cout << "Time difference impArray = " << std::chrono::duration_cast<std::chrono::nanoseconds>(end - begin).count() << "[ns]" << std::endl;
    std::copy(std::begin(arr), std::end(arr), out_it);
    std::cout << '\n';

    std::vector<int> v = {7804, 50398, 14945, 1814, 51383, 63156, 8432, 58103, 28175, 4339, 8361, 37158, 1529, 43066, 62052, 9591, 13168, 332, 55913, 2418, 48066, 46504, 52922, 39523, 36653, 30402, 9373, 56202, 50539, 41187, 42606, 32278, 63902, 41668, 7505, 46534, 25846, 49739, 63411, 45933, 15042, 6544, 35718, 17035, 34647, 15212, 52690, 64299, 61535, 45071};
    begin = std::chrono::steady_clock::now();
    etpc::sortBubble<int>(v);
    end = std::chrono::steady_clock::now();
    std::cout << "Time difference impVector = " << std::chrono::duration_cast<std::chrono::nanoseconds>(end - begin).count() << "[ns]" << std::endl;
    std::copy(std::begin(v), std::end(v), out_it);
    std::cout << '\n';

    return 0;
}
```

As you're already working with iterators you might as well accept iterators as arguments instead of the std::vector (as all STL algorithms do). That way it will work with other containers as well, even with raw pointers.

The comparison can also be customized like in std::sort. Maybe the caller wants to sort in descending order or compare structures by some field.


Count all possible attendance records with length n, which will be regarded as rewardable Given a positive integer n, return the number of all possible attendance records with length n, which will be regarded as rewardable. The answer may be very large, return it after \$\mod 10^9 + 7\$.

A student attendance record is a string that only contains the following three characters:

'A' : Absent.
'L' : Late.
'P' : Present.
A record is regarded as rewardable if it doesn't contain more than one 'A' (absent) or more than two continuous 'L' (late).

Link to this question on leetcode is here

I was able to come up with a recursive approach but could not memoize it like in DP. I want to use the same approach but with memoization. Below is my submitted code.

public class Solution {

    public long count = 0;
    public int checkRecord(int n) {

        func(1,n,0,0);
        return (int)count % (1000000007) ;

    }

    public void func(int pos , int n , int alreadyAbsent , int countOfLate)
    {
        if(pos > n)
        {
            count++;
        }
        else
        {
            func(pos + 1, n , alreadyAbsent,0);
            if(countOfLate < 2)
            {
                func(pos + 1,n,alreadyAbsent,countOfLate + 1);
            }
            if(alreadyAbsent == 0)
            {
                func(pos + 1,n,1,0);
            }
        }
    }
}

First of all, java doesn't like deep recursive calls. I tried running the following mini program on my machine and it only went up to 11398 before ending with a stackoverflow:

public static void main(String[] args) throws Exception {
    recurseTest(0);
}

public static void recurseTest(int n){
    System.out.println(n);
    recurseTest(n+1);
}

Notice that this only stores an int each call.

So you're better of calculating everything in a for loop instead.

It would be nice if we could do a single pass over the entire record choosing which letter to add at that step and update our total (+ helper totals). That way we don't even need to consider memoisation.

Let's look at which totals we want to keep track of to calculate the correct result.

First we have the requirement that the entire string can only have at most 1 letter A. So I would keep track of 2 groups of totals. Those that don't have an A yet. And those that do.

Next we must ensure that we can have at most 2 consecutive Ls. So let's keep track of how many L's are in the end at the previous step.

This leads to the following 6 variables.

long noA0;
long noA1;
long noA2;
long a0;
long a1;
long a2;

where noAx means it doesn't contain an A yet and ends with x letters L.

Here's what we would like to write in each step to update to the new values:

noA0 = noA0 + noA1 + noA2; //added P to any of the previous noA's
noA1 = noA0;
noA2 = noA1;
a0 = noA0 + noA1 + noA2 + a0 + a1 + a2; //added A to noA_ or added P to a_
a1 = a0;
a2 = a1;

But that has the problem that we are using updated values already. So we need to store some temp values instead.

tempnoA0 = noA0 + noA1 + noA2; //added P to any of the previous noA's
tempa0 = noA0 + noA1 + noA2 + a0 + a1 + a2; //added A to noA_ or added P to a_
noA2 = noA1;
noA1 = noA0;
a2 = a1;
a1 = a0;
noA0 = tempnoA0;
a0 = tempa0;

Now all you have to do is run this in a loop and figure out what the actual end result is. But that shouldn't be too hard right?

A last thing to consider is when to do the modulo. It may well be worth doing this at the right times during the loop as well to reduce the risk of overflowing the long (although I didn't actually look if this would be a real problem or not).


Setting Property Values Programmatically in a JSON Object Through the lens of the SOLID principles, which one of the following makes the most sense?  I want to ensure the approach I select keeps everything simple and maintainable as we add properties, modify or replace functions, and scale.  These objects may contain up to thirty properties or more and each one may require a value which changes.

Let's say I have a JSON object and I want to dynamically add values to it.

The scenarios below all technically work but I'm not sure of what the "best practice" would be when dynamically adding items in a JSON object, especially as it grows in the number of properties.

Scenario A) - Operations in a variable.

var added = 3 + 4;
var subtracted = 1 + 2;

var testObj = {
    key1: added,
    key2: subtracted
}

Scenario B) - Operations in a function.

var b = 1, c = 2, e = 3, f = 4;

function Added(x, y) ( return x + y }
function Subtracted(a, z) ( return a - z }

var testObj = {
    key1: Added(b, c)
    key2: Subtracted(e, f)
}

Scenario C) - Operations in the JSON properties.

var a = 1, b = 2, c = 3, d = 4;
var testObj = {
    key1: a + b
    key2: c - d
}

Is there another approach which I haven't yet considered?

Stupid nitpick..

There's no such thing as a JSON object in Javascript, they're just objects in Javascript. JSON stands for JavaScript Object Notation - Talking about JSON objects in the context of Javascript is like saying "strawberry flavored strawberry ice-cream which is strawberry flavored." Sorry. </rant>

Best option

If this is a realistic representation of your objective then the third one is the best, fastest and most concise. Creating a function to add two numbers is a waste of space and memory. Creating a variable is also a waste of memory as it will just have to get garbage collected.

Approaches you aven't considered

class add2Numbers{
    constructor(){
        this.addend = 0;
        this.augend = 0;
        this.result = 0;
    }

    parseNum(n){
        return parseFloat(n);
    }

    addAddend(n){
        this.addend = this.parseNum(n);
    }

    addAugend(n){
        this.augend = this.parseNum(n);
    }

    doMath(){
        this.result = this.addend + this.augend;
    }

    getResult(){
        return this.result;
    }
}

function insertProperty(obj, key, value){
    obj[key] = value;
}

var o = {};
var adder = new add2Numbers();
adder.addAddend(2);
adder.addAugend(5);
adder.doMath();
var result = adder.getResult();
insertProperty(o, 'sum', result);

// o now contains a property called 'sum' which contains the
// sum of the numbers 2 and 5
console.log(o);

</sarcasm>

The point that I'm trying to make is that new programmers and even old ones for some reason spend way too much time worrying about stupid buzzwords that they completely forget about common sense. The only buzzword you need right now is K.I.S.S.

Good luck and godspeed.


Project Euler #10 -- Summation of primes takes forever I have resolved the Problem presented in Project Euler #10:

The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.
Find the sum of all the primes below two million.

But this code takes too long before producing a result, 12.4195167 minutes to be exact.
What should I do to optimize this code?
public class summationOfPrimes{
public static void main(String args[]){
    long sum=0;
    outerloop:
    for(int i=2;i<2000000;i++){
        for(int j=2;j<=i;j++){
            if(i!=j&&i%j==0){
                continue outerloop;
            }
            else if(j==i){
                sum=sum+i;
            }
        }

    }

    System.out.println(sum);

}

}

Well, let's start with a few optimizations for you:

Any i that is a multiple of 2 but not 2, is not a prime. Consider upping i by 2 at a time, and starting sum off at 2. i would start at 3.
If something is divisible by 4, it's divisible by 2. Therefore, it's a good idea to make j iterate through a list of already found primes - it's no use testing if something is divisible by 4 if it's not divisible by 2.
3*3 = 9. When you are checking whether a certain i is a prime, you can stop at the point where j*j is greater than the prime to test. For 7, you can stop at 3; 3*3 = 9, after all.


Data Structures for Counting Duplicates and using std::vector::erase Problem
Dupe detection for a vector of ints. I simply want a count of the unique input characters that occurred at least twice. The goal is to count a dupe only once and ignore that input character if another dupe of it is seen in the future. A test input could look something like this vector<int> test = { 4,5,9,6,9,9,6,3,4 };
Looking for Feedback on
Looking for basic feedback on the data structures I'm using and the possibility of using the vector erase method to iterate and take advantage of the space allocated to my numbers vector instead of using a map to not count dups more than once. Any C++ 11 or 17 features I can take advantage of here too?
int countDuplicates(vector<int> numbers) {
    int dups = 0;
    set<int> s;
    map<int, int> m;
    for (int n : numbers) {
      if (s.insert(n).second == false && m.find(n) == m.end()) {
          dups++;
          m.insert(pair<int, int>(n,0));
          // better to remove from vector than increase space with the map?
          // numbers.erase(remove(numbers.begin(), numbers.end(), n), numbers.end());
        } else {
          s.insert(n);
        }
    }

    return dups;
}

Basic Algorithm
At least if I understand the intent correctly, you simply want a count of the unique input characters that occurred at least twice.
In that case, I think I'd do something like this:
int count_dupes(std::vector<int> const &inputs) {
    std::map<int, int> counts;

    for (auto i : inputs)
        ++counts[i];

    return std::count_if(counts.begin(), counts.end(),
                         [](auto const &p) { return p.second >= 2; });
}

I'd also consider using an array instead of a map, as outlined in an answer to an earlier question: https://codereview.stackexchange.com/a/208502/489 --but this can depend on the range of values you're dealing with. With a 16-bit int, it's no problem at all on most machines. With a 32-bit int (and no other constraints on values) it's still possible on many machines, but probably impractical. For arbitrary 64-bit int, an array won't be practical.
Parameter Passing
Right now, you're passing the input by value. This means when you call the function with some vector, a copy of the original vector will normally be made and passed to the function. As a general rule, something like a vector that's potentially large and slow to copy should be passed by reference to const, as shown in the code above.
Logical Comparisons
Comparing a Boolean value to true or false is generally a poor idea. if (x==true) is equivalent to if (x) and if (x == false) is equivalent to if (!x). Normally, if it's Boolean in nature, a variable should be given a name that reflects that nature, and should be used directly rather than being compared to true or false. For example, s.insert(n).second == false wold be better written as: if (!s.insert(n).second).
Some people (understandably, I guess) prefer to use the written form: if if (not s.insert(n).second). I've written C and C++ long enough that I have no difficulty with reading ! as meaning "not", but especially if it may be read by people less accustomed to programming, it may make more sense to use the words instead of symbols.
Formatting/Indentation
At least to me, this indentation looks a bit odd:
  if (s.insert(n).second == false && m.find(n) == m.end()) {
      dups++;
      m.insert(pair<int, int>(n,0));
      // better to remove from vector than increase space with the map?
      // numbers.erase(remove(numbers.begin(), numbers.end(), n), numbers.end());
    } else {
      s.insert(n);
    }

If you use indentation like that consistently, I guess it's not necessarily terrible, but I think more people are accustomed to something more like this:
  if (s.insert(n).second == false && m.find(n) == m.end()) {
      dups++;
      m.insert(pair<int, int>(n,0));
      // better to remove from vector than increase space with the map?
      // numbers.erase(remove(numbers.begin(), numbers.end(), n), numbers.end());
  } else {
      s.insert(n);
  }

...where each closing brace is vertically aligned with the beginning of the block it closes. As a side-note, there are almost endless debates about the efficacy of various bracing styles. I'm not going to advocate for or against any of the well known styles, but I think there's a fair amount to be gained from using a style that's well known, and then using it consistently. I don't see much to gain from style that's different from what almost anybody else uses.


Convert string to multiline text I made this method that takes any string and transforms it into a multiline text l; each line having the max (character) length specified by the rowLength parameter.

How can I make this better, functionality and code wise?

There is currently one known issue. It sometimes adds an empty line at the end because I have to add the Enviroment.NewLine call. Check it out, please, and give some suggestions.

public static string ConvertToMultiLineText(string value, int rowLength)
{
    var multiLine = new StringBuilder();
    string[] lines = value.Split(new[] { Environment.NewLine }, StringSplitOptions.None);
    var filterLines = lines.Where(line => !line.IsNullEmptyOrWhiteSpace());

    foreach (var line in filterLines)
    {
        if (line.IsNullEmptyOrWhiteSpace())
            continue;

        if (line.Length > rowLength)
        {
            int startIndex = 0;
            var number = (decimal)line.Length / rowLength;
            var rowNumber = Math.Ceiling(number);

            for (int i = 0; i < rowNumber; i++)
            {
                if (line.Contains(@" "))
                {
                    int lastIndex = rowLength;

                    // if it's not the first row set the last character on the current row of the iteration
                    if (i != 0)
                        lastIndex = (rowLength * (i + 1));

                    // if it's the last row set last character as the lenght of the last row
                    if (i == rowNumber - 1)
                        lastIndex = line.Length - 1;

                    int lastSpace = line.LastIndexOf(@" ", lastIndex, StringComparison.Ordinal);

                    //if " " is found inside the string
                    if (lastSpace > -1)
                    {
                        var lastOccurence = (lastSpace <= startIndex) ? startIndex - lastSpace : lastSpace - startIndex;

                        if (i == rowNumber - 1)
                            multiLine.Append(line.Substring(startIndex, lastOccurence) + Environment.NewLine + line.Substring(lastSpace, line.Length - lastSpace));
                        else
                            multiLine.Append(line.Substring(startIndex, lastOccurence) + Environment.NewLine);

                        startIndex = lastSpace;
                    }
                    else
                    {
                        if (i == rowNumber - 1)
                            multiLine.Append(line.Substring(startIndex, (line.Length - rowLength * ((int)rowNumber - 1))));
                        else
                            multiLine.Append(line.Substring(startIndex, rowLength) + Environment.NewLine);
                        startIndex += rowLength;
                    }
                }
                else
                {
                    if (i == rowNumber - 1)
                        multiLine.Append(line.Substring(startIndex, (line.Length - rowLength * ((int)rowNumber - 1))));
                    else
                        multiLine.Append(line.Substring(startIndex, rowLength) + Environment.NewLine);
                    startIndex += rowLength;
                }
            }
        }
        else
            multiLine.Append(line.Substring(0, line.Length) + Environment.NewLine);
    }

    return string.Format(@"{0}", multiLine);
}

First Impressions
I haven't gone through the functionality in detail but a few things strike me immediately.

Why explicitly include StringSplitOptions.None. They default value is none.

IsNullEmptyOrWhiteSpace() seems to be a (not included) extension method for string that performs the same function as String.IsNullOrWhiteSpace.  Does it do something else as well?

The check for an empty 'line' inside the loop

if (line.IsNullEmptyOrWhiteSpace())
        continue;

should be redundant as we have previously filtered the lines

var filterLines = lines.Where(line => !line.IsNullEmptyOrWhiteSpace());

I would need to do up some performance checks to be sure but I reckon that the use of the StringBuilder (presumably for performance reasons) will be negated by the calls to line.SubString().

I can't tell for certain what the requirements are  - I would guess

Max line Length of n
Do not split words
Do not end on a space
Do not start on a space

but it look like a linear progression through the string, with some back tracking to deal with split words/spaces, might work and appending the individual chars might give you better performance.

The final return should be

return multiLine.ToString();

there is no need to create another string and place the result of multiLine.ToString() inside it and then return the anonymous string.


Split string in half and change case accordingly I found the following challenge online:

Create a function that takes a string and returns that string with the first half lowercased and the last half uppercased.
eg: foobar == fooBAR
If it is an odd number then 'round' it up to find which letters to uppercase.

That sounded easy enough but I found the last requirement a little tricky. Eventually this is what I came up with:

def sillycase(silly):
    length=len(silly)
    firstHalf = silly[:length / 2 + (length % 2)].lower()
    secondHalf = silly[length / 2 + (length % 2):].upper()
    return firstHalf + secondHalf

but that seems repetitive and inefficient.
What could I have done to make it slicker, without sacrificing readability? I was actually a little surprised python didn't have a built in method to split a string in half.

Midpoint math is often a pain in the whatnot, but a real trick is to add 1 to the length before halving it. Additionally, instead of repeating the math multiple times, you can do it just once.

def sillycase(silly):
    mid=(len(silly) + 1) / 2
    firstHalf = silly[:mid].lower()
    secondHalf = silly[mid:].upper()
    return firstHalf + secondHalf

At that point, it becomes small enough to not need the temp variables:

def sillycase(silly):
    mid = (len(silly) + 1) // 2
    return silly[:mid].lower() + silly[mid:].upper()

print (sillycase("hello"))
print (sillycase("helloa"))
print (sillycase("helloab"))

http://ideone.com/CqUdl5

Note the use of the integer divide which is suitable for both python 2.x and 3.


Cleaning up nested promises in Cloud function I am writing the Cloud function for Parse whose purpose is to update my database with a provided array of data objects.  The main problem are the nested promises, which doesn't look right.  Any help to refactor this code or any other critiques?

/**
 * Updating multiple object arrays:
 * Look for object with the same idKey and update with supplied data or create new if none is found
 * Expect Data {schema: <schema_name>, idKey: <name_of_id_key>, data: <object_array>}
 */

Parse.Cloud.define("updater", function(request, response) {
    var _ = require('underscore');

    var schema          = request.params.schema,
            idKey       = request.params.idKey,
            objectArray = request.params.data;

    var Class           = Parse.Object.extend(schema),
            parseObject,
            id,
            query;

    _.each(objectArray, function(dataObject){
            id = dataObject[idKey];
            if (!id) return response.error("Supplied Object " + dataObject + " has no id Key '" + idKey + "' set!");

            // now query for the same idKey
            query           = new Parse.Query(Class);
            query.equalTo(idKey, id);
            query.find().then(
                function(array){
                    if(array) {
                        // found at least one object - take the first
                        parseObject = array[0];

                        // use the new dataObject to update parseObject
                        parseObject.set(dataObject);
                        parseObject.save().then(

                            /*
                             * should this go outside the promise?
                             */

                            function(object){
                                // success
                            },
                            function(error){
                                return response.error(error);
                            });
                    } else {
                        // nothing found - create new
                        parseObject = new Class(dataObject);
                        parseObject.save().then(
                            function(object){
                                // success
                            },
                            function(error){
                                return response.error(error);
                            });
                    }
                }
            );

            parseObject = new Class(dataObject);

            parseObject.save().then(
            function(object){
                // success
            },
            function(error){
                response.error(error);
            });
    });

    response.success("Success!");

});

I'd start with refactoring the promises functions and remove any code repetition, something like this:

...

function success(object){
    // success
}
function error(error){
    return response.error(error);
}
...

query.find().then(function(array){
    if(array) {
        // found at least one object - take the first
        parseObject = array[0];
        parseObject.set(dataObject);
     } else {
        // nothing found - create new
        parseObject = new Class(dataObject);
     }
     parseObject.save().then(success,error);
});
...


MySQL request in Python In my code I have three requests to a MySQL database:

@app.route('/private', methods=['POST'])
def private():
    login = request.form['login']
    if login is None or not login:
        return jsonify(data='Incorrect URL')

    try:
        c, conn = cursor_connection()
        c = conn.cursor()
        c.execute("SELECT accounts_info_uid "
                  "FROM auth_info WHERE login='{}' ".format(login))

        id = c.fetchall()
        if not id:
            return jsonify(data='Incorrect login')

        c.execute("SELECT * FROM boxes_id AS tb1 LEFT JOIN"
                  " accounts_info AS tb2 ON tb2.boxes_ids=tb1.uid "
                  # "LEFT JOIN electricity_info as tb3 ON tb3.boxes_id_uid=tb1.uid"
                  " WHERE tb2.uid={} ".format(id[0][0]))

        uid, mc_address, working_status, activation_status, _,\
        first_name, second_name, registration_date, phone, email, boxes_id = c.fetchall()[0]
        c.execute(" SELECT consumed_electricity "
                  "FROM electricity_info "
                  "WHERE boxes_id_uid={} ".format(boxes_id))
        consumed_electricity = [float(val[0]) for val in c.fetchall()]
        c.close()
        conn.close()

    except Exception as e:
        logger.error(msg='Cannot execute /private {}'.format(e))
        return str(e)

I fetched a list from electricity info by primary key in boxes_id (so in electricity_info it is called boxes_id_uid).

Structure of pk in my tables:

auth_info --------> pk is accounts_info_uid

boxes_id ----------> pk is uid

accounts_info ------> pk is uid and it is connected to table boxes_id by field boxes_id

electricity_info ------> pk is boxes_id_uid

I think it can be optimized in one SQL request. If so, can you tell me how to achieve that?

DONT! String format your sql queries, but let the cursor do it for you.

As @Gareth Rees said, this is not secure!

c.execute("SELECT accounts_info_uid "
          "FROM auth_info WHERE login='{}' ".format(login))

As taken from the docs, this would be the proper way to execute statements with sql:

cursor.execute("SELECT accounts_info_uid FROM auth_info WHERE login = %s", login)

You could use a context manager over the connection

Using a context manager would improve the readability, see PEP343

There are some resource out there how to implement such a thing
Make the login a separate function
I think it can be optimized in one SQL request?

Yes, but without the database itself it is hard to test, a github repo or some more information would help.

However you could improve on your google-fu, Simply typing "mysql join 3 tables" would have given you the answer you are looking for.

https://stackoverflow.com/questions/3709560/joining-three-tables-using-mysql


Getting funky with Pi Push a button and an LED lights up1 and sends a signal to another computer that plays a song.

First project on the Raspberry pi, really simple but definitely fun and educational to implement. Also, second time using python and sockets so I'd especially appreciate input on anything I do inefficiently.2

Mostly feedback to ensure that the program is running/button is active.
I know it's straightforward, but this site has shown me that even the simplest things leave room to be executed much more elegantly.

SongServer.java:

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Date;
import java.awt.Desktop;

public class SongServer {
    final static int PORT = 9001;

    public static void main(String[] args) {
        try(ServerSocket server = new ServerSocket(PORT)) {
            System.out.println(new Date() + "\nServer online.");

            while(true) {
                new Thread(new ClientHandler(server.accept())).start();
            }
        } catch (IOException ioe) {
            ioe.printStackTrace();
        }
    }

    private static class ClientHandler implements Runnable {
        private Socket socket;
        private BufferedReader in;
        private PrintWriter out;

        ClientHandler(Socket socket) {
            this.socket = socket;
            System.out.println("Client connected!");
        }

        @Override
        public void run() {
            try {
                in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
                out = new PrintWriter(socket.getOutputStream(), true);

                out.println("Connection success");
                while (true) {
                    String input = in.readLine();
                    if (input == null || input.trim().isEmpty()) {
                        continue;
                    }
                    System.out.println(input);

                    if (input.startsWith("play")) {
                        Desktop.getDesktop().open(new File("uptown_funk.mp3"));
                    }
                }
            } catch (Exception e) {
                System.out.println("Client disconnected!");
            }
        }
    }
}

funk_button.py:

import RPi.GPIO as GPIO
import socket

funk_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
funk_socket.connect(("10.0.0.18", 9001))
sent = False

button = 17
led = 22

GPIO.setwarnings(False)
GPIO.setmode(GPIO.BCM)
GPIO.setup(led, GPIO.OUT)
GPIO.setup(button, GPIO.IN)

while True:
    if GPIO.input(button):
        GPIO.output(led, GPIO.LOW)
    else:
        GPIO.output(led, GPIO.HIGH)
        if not sent:
            funk_socket.send(bytearray("play\r\n", "utf-8"))
            sent = True

Looks good as a proof of concept. I recommend to modularize python code, that is

Make socket interface into a function
Make GPIO interface into a function
Have a

if __name__ == '__main__'

clause to call them (directly or via a main) function.

I would also handle failures to connect (retry maybe?); a total fun is to implement service discovery via pybonjour or zeroconf.


Currency converter in Python 2.7 I'm a beginner and it is my first project in Python 2.7.

My goal is writing a script that I can run with two or three arguments.
If run with two arguments like python currency.py RUB USD then the output will be current RUB rate by USD base: .015169.
If run with three arguments like python currency.py 1000 RUB USD then it will convert 1000 RUB to dollar.

#!/usr/bin/env python
import ast, requests
import sys, argparse
def checker(base, symbol):
    url = "http://api.fixer.io/latest?base=%s" % base.upper()
    r = requests.get(url)
    content = ast.literal_eval(r.content)
    rate = content["rates"][symbol.upper()]
    return rate
parser = argparse.ArgumentParser(description = "Currency converter. For instance : 'Convert 100 USD to RUB' Will be: 100 USD RUB. If You need only rate: USD RUB")
if len(sys.argv) > 3:
    parser.add_argument("n", type =int, help = "Amount of currency_for_convert", default=1)
parser.add_argument('currency_for_convert', type=str, help="Currency for convert")
parser.add_argument("currency_converted_to", type=str, help = "Currency that converted to")
args = parser.parse_args()
rate = checker(args.currency_for_convert, args.currency_converted_to)
print rate if len(sys.argv) <= 3 else rate*args.n

But I don't like the way I'm using if statements because the running script help ignore third argument and give information only about first two arguments.

  $ python currency.py --help usage: currency.py [-h]
  currency_for_convert currency_converted_to

  Currency converter. For instance : 'Convert 100 USD to RUB' Will be:
  100 USD RUB. If You need only rate: USD RUB

  positional arguments:   currency_for_convert   Currency for convert
  currency_converted_to  Currency that converted to

  optional arguments:   -h, --help            show this help message and
  exit

Is there another way to write two/tree arguments script?

Never assume that your user doesn't make mistakes.  I ran your program with no arguments ... error message, fine.  I ran it with a single argument of 4 ... error message, fine.  I ran it with two arguments, 4 and 5 ...:

Traceback (most recent call last):
  File "test.py", line 17, in <module>
    rate = checker(args.curency_for_convert, args.curency_converted_to)
  File "test.py", line 9, in checker
    rate = content["rates"][symbol.upper()]
KeyError: 'rates'

For debugging purposes, I put print content above that line and ran again:

{'error': 'Invalid base'}

If I run the program with a valid base, but an invalid symbol:

Traceback (most recent call last):
  File "test.py", line 16, in <module>
    rate = checker(args.curency_for_convert, args.curency_converted_to)
  File "test.py", line 8, in checker
    rate = content["rates"][symbol.upper()]
KeyError: '8'

You should catch these errors and display something that is a little more user-friendly.

I would recommend that you read PEP 8, the Python style guide.  There are a couple things you aren't following:

  Imports should usually be on separate lines, e.g.:

  Yes:

import os
import sys

  No:

import sys, os

  It's okay to say this though:

from subprocess import Popen, PIPE

  Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.

Your whitespace is a little strange.  I would have at least one blank line above and below the function definition.  I would also put one after the if block and maybe even before it too.  You have " =", " = ", and "=" used in just three lines.  PEP 8 says that A Foolish Consistency is the Hobgoblin of Little Minds, but that doesn't mean never be consistent.  It is only the foolish consitency which is a hobgoblin.  In this case, you have nothing to lose.

Your naming is a little strange.  n even shows up in the help messages.  One-letter variable names are generally not recommended because they give very little idea of what the variable is for.  curency_for_convert and curency_converted_to, besides mis-spelling "currency", are ungrammatical.  You could say "for conversion" or "to convert", but "for convert"?  I would suggest "convert_from" and "convert_to".

if len(sys.argv) > 3:
    parser.add_argument("n", type =int, help = "Amount of curency_for_convert", default=1)

Instead of manually parsing an argument, make it optional with nargs='?'

print rate if len(sys.argv) <= 3 else rate*args.n

Since our argument will always be present (either with the default of 1 or with a user-supplied number), we can now do print rate * args.n.

Full Program

#!/usr/bin/env python
import argparse
import ast
import requests
import sys

def checker(base, symbol):
    url = "http://api.fixer.io/latest?base=%s" % base.upper()
    r = requests.get(url)
    content = ast.literal_eval(r.content)
    rate = content["rates"][symbol.upper()]
    return rate

parser = argparse.ArgumentParser(description=
    "Currency converter. For instance: to convert 100 USD to RUB, it would be "
    "100 USD RUB. If you need only the rate: USD RUB"
)

parser.add_argument("amount", type=int, help="Amount of currency to convert", default=1, nargs='?')
parser.add_argument('convert_from', type=str, help="Currency to convert")
parser.add_argument('convert_to', type=str, help = "Currency of converted")

args = parser.parse_args()

try:
    rate = checker(args.convert_from, args.convert_to)
except KeyError:
    print "Invalid argument(s)"
else:
    print rate*args.amount


Scroll listener - about code style guidelines and standards I'm an upstart, been only professionally programming for a bit less than 2 years, mainly in Java as I am currently working as an Android developer.

I often find myself thinking about how to improve the readability and maintainability of my code, and establish some style guidelines, for the project that I am working on, but also in general. On this project, I am using git and expect to have a few more developers joining me in the near future, and would like to keep our code style as readable/consistent/easily maintainable as possible. I have the (rare, I'm told) opportunity to establish some guidelines and create some configuration files for the code formatting which would then be used team-wide and keep things consistent.

I guess I want to:

keep the lines relatively short, preferably less than 80 characters (e.g. coding on an ultrabook with a 14 inch display with excessively long lines is not fun)
avoid refactoring variable/method names resulting in misalignment of method parameters
keep chained method calls readable
keep complex logical conditions readable
keep the list of implemented interfaces readable
Any other suggestions?

I have some ideas on how to accomplish this, but would like to hear if there's something that I can improve. Here's an example of a method which features points 2, 3, 4:

@Override
public void onScroll(
    AbsListView view,
    int firstVisibleItem,
    int visibleItemCount,
    int totalItemCount
) {
    super.onScroll(
        view,
        firstVisibleItem,
        visibleItemCount,
        totalItemCount
    );
    if(mUpdateFixedHeader &&
       !(
           mAdapter.isFiltered() &&
           mAdapter.isEmpty()
       ) &&
       mDateHeader != null
    ) {
       mDateHeaderTextView.setText(
           mDateHeaderLabel.concat(
              ((MeetsListAdapter) mAdapter)
                  .getSectionForPosition(firstVisibleItem)
           )
       );
    }
}

I'm not too sure about the way I've chained the logical conditions here, and I wanted to have the opening curly braces in a new lines but felt it makes the whole thing very disjointed. I would do it for the opening braces of a class though.

And, for a class declaration example, for a class that implements several interfaces, I'm thinking something along the lines of this (point 5):

public abstract class AbstractListFragment<T>
    extends
        AbstractFragment
    implements
        OnItemClickListener,
        OnScrollListener,
        SearchListener
{
    ...
}

Looking forward to your thoughts/suggestions!

Code style guidelines are available from a number of places. The 'old faithful' official Java ones have been pulled down, for some reason. Oh, they have been restored and are available in an archive format.... There are two other references that are commonly used for Java code style:

Original Java code style - Archived.
Google's Java code style
Eclipse/IntelliJ/Netbeans default formats (built-in guides).

This StackOverflow question seems particularly relevant: Official Java Code Guidelines/Conventions.

What all of these guides have in common is, and is contrary to your code, is:

don't put each parameter to the method on its own line. The limiting factor is the number of characters, not the number of parameters, on the line. This is true for both when you declare the method, and call the method.
never start a line with a close-parenthesis ). The close-parenthesis goes at the end of the previous line.
conditions in an if-block should be grouped where convenient, and line-breaks, if any, should be at logical breaks in the conditions. On the other hand, you are too generous with line-breaks in your conditions... !( on it's own line is bizarre.
when splitting long lines (string constants, long conditionals), put the 'operator' at the start of the next line, not the end of the previous one. For example, you have:

 mAdapter.isFiltered() &&
 mAdapter.isEmpty()

and that should be:

  mAdapter.isFiltered()
  && mAdapter.isEmpty()

Really, there are only a few areas of real controversy in Java code style guidelines:

indentation using tabs, or spaces: all code-style guidelines I have read for Java indicate that spaces should be used. For example, the Official (old) Java spec: "Four spaces should be used as the unit of indentation", and the Google spec "Each time a new block or block-like construct is opened, the indent increases by two spaces."

For some reason (and I am not sure why), a number of IDE's ship using tabs as the default indentation mechanism (I'm looking at you, Eclipse).

The amount of indentation is often site-specific, whether 2 or 4 spaces. Personally I am happy with either (as long as it is consistent), but I prefer 4.

Tabs vs. Spaces can become a battle of cultures, and it is seldom resolved without someone feeling upset.... but, when working as a team, using a version-control-system, you have to decide on one system, and then you have to be consistent with all developers. Messing with indentation after the fact is a very messy process in a version-control system.
End-of-line open-brace {, or new-line brace: this is one of those style things that is hotly debated, but Java conventions are clear. Brace at the end of the line. The thing is, that many other languages recommend a brace on the next line, especially 'punctuated' C languages like C, C++, C#. Because people often write in multiple languages and they like their habits to be the same in both, they often code Java with a brace on the next line. For Java, this is wrong though, even if it is inconvenient for those folk. Java has lines like:

if (condition) {
    .... do something....
}

the C* languages put the brace on the new line:

if (condition)
{
    .... do something....
}

Let's be clear on this one. A lot of people use newline-brace, but, in Java, that is not right, even if it is somewhat common.

Further, there are arguments for, and against the brace style, but it's a pointless argument. As much as there may be meaningful arguments about one, or the other, the consistency of usage is more important than any one argument.
indentation of switch-blocks: for some reason, there is a lot of inconsistency in switch-block indentation. I prefer the case/default blocks to be indented 4 from the switch, and the code for the case indented 4 from the case (Google style agrees with this indentation of the case, but uses 2 spaces). For example:

switch (somefowl) {
    case DUCK:
        System.out.println("Duck!");
        break;
    case GOOSE:
        System.out.println("Goose!");
        break;
    default:
        System.out.println("What?");
}

Many people, and some IDE defaults, do not indent the case/default (and the original Java guidelines use this style), and the code looks like:

switch (somefowl) {
case DUCK:
    System.out.println("Duck!");
    break;
case GOOSE:
    System.out.println("Goose!");
    break;
default:
    System.out.println("What?");
}

It's not a big deal. But, again, be consistent.
Braced 1-liners: One additional controversy is braces around 1-liners. You often see code like:

if (badDog)
    return "Sit! Stay!";

all code style guidelines recommend using braces even on 1-liners, and the code should look like:

if (badDog) {
      return "Sit! Stay!";
}

Google has this to say:

  Braces are used with if, else, for, do and while statements, even when the body is empty or contains only a single statement

The Java style guide says this:

  Note: if statements always use braces {}. Avoid the following error-prone form:

if (condition) //AVOID! THIS OMITS THE BRACES {}!
    statement;

In general I use Eclipse. The best thing for consistency in a team is for the whole team to use the same IDE for their code, and to install the same code-style 'rules'. Then, it becomes 'easy', you just type (in eclipse): Ctrl-A and Ctrl-Shift-F. Doing that, with your code, in my Eclipse, produces:

@Override
public void onScroll(AbsListView view, int firstVisibleItem,
        int visibleItemCount, int totalItemCount) {
    super.onScroll(view, firstVisibleItem, visibleItemCount, totalItemCount);
    if (mUpdateFixedHeader
            && !(mAdapter.isFiltered() && mAdapter.isEmpty())
            && mDateHeader != null) {
        mDateHeaderTextView.setText(mDateHeaderLabel
                .concat(((MeetsListAdapter) mAdapter)
                        .getSectionForPosition(firstVisibleItem)));
    }
}

That looks about right, to me. No change needed.

When I auto-format your class, I get:

public abstract class AbstractListFragment<T> extends AbstractFragment
        implements OnItemClickListener, OnScrollListener, SearchListener {
    ...
}

Which also looks right to me.

Finally, for the above code formatting, I 'turned off' the Eclipse option to 'don't join lines'. Eclipse will 'collapse' lines when auto formatting. I often add additional empty lines in my code to make the code look like paragraphs. I want the auto-formatter to keep these empty lines, so I tell it not to format them out.

If I were to write this code, I would add some 'paragraphs' in to the code manually, and it would look like:

@Override
public void onScroll(AbsListView view, int firstVisibleItem,
        int visibleItemCount, int totalItemCount) {

    super.onScroll(view, firstVisibleItem, visibleItemCount, totalItemCount);

    if (mUpdateFixedHeader
            && !(mAdapter.isFiltered() && mAdapter.isEmpty())
            && mDateHeader != null) {

        mDateHeaderTextView.setText(mDateHeaderLabel
                .concat(((MeetsListAdapter) mAdapter)
                        .getSectionForPosition(firstVisibleItem)));

    }
}


Dynamic ranges and page-formatting I've put together the code below, which creates a new sheet in my workbook and applies dynamically-named ranges and page-formatting.

Sub UniqueOverheads()

Set rOverheadsList = Range([B4], [B4].End(xlDown))
Set rOverheadsActuals = Range([C4], [C4].End(xlDown))
Set rOApr = Range([D4], [D4].End(xlDown))
Set rOMay = Range([E4], [E4].End(xlDown))
Set rOJun = Range([F4], [F4].End(xlDown))
Set rOJul = Range([G4], [G4].End(xlDown))
Set rOAug = Range([H4], [H4].End(xlDown))
Set rOSep = Range([I4], [I4].End(xlDown))
Set rOOct = Range([J4], [J4].End(xlDown))
Set rONov = Range([K4], [K4].End(xlDown))
Set rODec = Range([L4], [L4].End(xlDown))
Set rOJan = Range([M4], [M4].End(xlDown))
Set rOFeb = Range([N4], [N4].End(xlDown))
Set rOMar = Range([O4], [O4].End(xlDown))

    Worksheets.Add(after:=Worksheets(4)).Name = "Overheads"

    Range("B3").Select
    ActiveCell.FormulaR1C1 = "Overheads Code"
    With Selection.Interior
        .ColorIndex = 37
        .Pattern = xlSolid
    End With
    Selection.Font.Bold = True
    Cells.Select
    With Selection.Font
        .Name = "Lucida Sans"
        .Size = 10
        .Strikethrough = False
        .Superscript = False
        .Subscript = False
        .OutlineFont = False
        .Shadow = False
        .Underline = xlUnderlineStyleNone
        .ColorIndex = xlAutomatic
    End With

    ActiveWorkbook.Names.Add Name:="OverheadsList", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOverheadsList.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OverheadsActuals", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOverheadsActuals.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OApr", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOApr.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OMay", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOMay.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OJun", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOJun.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OJul", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOJul.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OAug", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOAug.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OSep", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOSep.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OOct", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOOct.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="ONov", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rONov.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="ODec", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rODec.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OJan", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOJan.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OFeb", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOFeb.Address(ReferenceStyle:=xlR1C1)

    ActiveWorkbook.Names.Add Name:="OMar", RefersToR1C1:="=" & _
    ActiveSheet.Name & "!" & rOMar.Address(ReferenceStyle:=xlR1C1)

End Sub

The code does work, but I'm a little concerned that it may be a little clunky and could be written smarter. I'm relatively new to VBA but I'm willing to learn.  I'm wondering if someone, who is perhaps a more seasoned programmer than myself, could look at this.  Please offer some guidance on how I may be able to write this a little better.

Try to avoid using Select. I have fixed that for you.

This line: Range(Cells(4, i + 2), Cells(Cells(Rows.Count, i + 2).End(xlUp).Row, i + 2)).Address(ReferenceStyle:=xlR1C1)

may require you to add Sheets("SheetName") separated by dot . before Range to specify the sheet you want the data to refer to.

Also added Columns.Autofit at the end to automatically resize column widths upon completion

Sub UniqueOverheads()

    Worksheets.Add(after:=Worksheets(4)).Name = "Overheads"

    With Sheets(Sheets.Count).Range("B3")
        .Value = "Overheads Code"
        .Interior.ColorIndex = 37
        .Interior.Pattern = xlSolid
        .Font.Bold = True
    End With
    With Sheets(Sheets.Count).Cells.Font
        .Name = "Lucida Sans"
        .Size = 10
        .Strikethrough = False
        .Superscript = False
        .Subscript = False
        .OutlineFont = False
        .Shadow = False
        .Underline = xlUnderlineStyleNone
        .ColorIndex = xlAutomatic
    End With

    Dim names As Variant
    names = Array("OverheadsList", "OverheadsActuals", "OApr", "OMay", "OJun", "OJul", "OAug", "OSep", _
                  "OOct", "ONov", "ODec", "OJan", "OFeb", "OMar")

    For i = LBound(names) To UBound(names)
         ActiveWorkbook.names.Add Name:=names(i), RefersToR1C1:="=" & _
         ActiveSheet.Name & "!" & _
         Range(Cells(4, i + 2), Cells(Cells(Rows.Count, i + 2).End(xlUp).Row, i + 2)).Address(ReferenceStyle:=xlR1C1)
    Next i
    Columns.AutoFit
End Sub


Netty-based safely network reconnection I am writing a TCP client app using netty

NettyConnectionManager.scala:

class NettyConnectionManager(val config : NettyConfig) extends ConnectionManager {
  @volatile private var connection: Option[Connected] = None

  def get() = {
    if(isConnected) {
      connection.get
    } else {
      synchronized {
        if(isConnected) {
          connection.get
        } else {
          val newConnection = new NettyConnection(config)
          newConnection.connect()
          connection = Some(newConnection)
          connection.get
        }
      }
    }
  }

  private def isConnected = {
    (for(c <- connection if c.isConnected) yield true).getOrElse(false)
  }

  def close() {
    synchronized {
      if(isConnected) {
        connection.foreach(_.close())
      }
    }
  }
}

NettyConnection:

class NettyConnection(val config: NettyConfig) extends Connection {

  @volatile
  private var channel = None: Option[Channel]

  def connect() = {
    val channelFuture = bootstrap().connect(new java.net.InetSocketAddress(config.hostname, config.port))
    channelFuture.awaitUninterruptibly
    channel = Some(channelFuture.getChannel)
  }

  def isConnected = channel.map(_.isOpen).getOrElse(false)
  ...

I am trying to do auto-reconnect in NettyConnectionManager.

Now I have three questions:

Is channel.isOpen method reflect the channel states immediately?
Is NettyConnectionManager thread safe? (get method never return a closed connection more than one time).
Is there a better way doing such reconnect job?

A netty Channel can be open and not connected so your NettyConnection.isConnected is not correct. See this.
and 3.

It's pointless to write such code in scala. What you need is just a scala lazy val. Try this:

class NettyConnectionManager(val config: NettyConfig) extends ConnectionManager {
  lazy val connection = {
    val newConnection = new NettyConnection(config)
    newConnection.connect()
    newConnection
  }

  def close = connection.close
  def connected = connection.isConnected
}

scala lazy vals already use double check locking to initial the val only once, you don't have to do all the primitive stuff yourself as in NettyConnectionManager or NettyConnection. Next time you think about using var of Option, use lazy val instead. See SIP-20 to know more about how scala community is trying to improving this strategy.

And, wrapping a Channel in two classes like NettyConnection and NettyConnectionManager is also bad design decision. You gain little and add much unnecessary complexity.


Correct implementation of encryption function I wrote a simple encryption function that uses argon2d as the KDF and XChaCha20-Poly1305 as the AEAD. Since it is very easy to not get things right when dealing with encryption, I would appreciate if someone could review my script and tell me if it has any vulnerabilities and could be used in the real world to encrypt real data.
import argon2
from Cryptodome.Cipher import ChaCha20_Poly1305
import secrets

def encrypt(password, data):
    salt = secrets.token_bytes(32)
    nonce = secrets.token_bytes(24)

    key = argon2.low_level.hash_secret_raw(secret=password, salt=salt, time_cost=3, memory_cost=1000000, parallelism=4, hash_len=32, type=argon2.low_level.Type.D, version=19)

    cipher = ChaCha20_Poly1305.new(key=key, nonce=nonce)

    ciphertext, tag = cipher.encrypt_and_digest(data)

    return salt + nonce + ciphertext + tag

You can add annotations in function parameters, like password: str and so on.


LINQ query that filters elements from a list of object I have two lists of objects Person and PersonResult. Both are linked through the property PersonId. I need to create a filter for the list of PersonResult that meet certain criteria for the Person (e.g. Person.Gender == "female").

Im currently using the following LINQ query to achieve this:

PersonResultList = PersonResultList.Where(pr =>
    PersonList.FirstOrDefault(p => pr.PersonId == p.PersonId) != null &&
    PersonList.FirstOrDefault(p => pr.PersonId == p.PersonId).Gender == "female");

This works apparently well, however, I must iterate twice through PersonList to check if the person exist and its gender. Is there a more elegant way to achieve this?

You can simply combine the conditions inside the FirstOrDefault() like

PersonResultList = PersonResultList
    .Where(pr => PersonList
                    .FirstOrDefault(p => pr.PersonId == p.PersonId
                                         && p.Gender == "female") != null );

Because I only changed your existing code, it didn't came to my mind what Nikita Brizhak commented here .

  You should probably use Any instead of FirstOrDefault

So let us change the code to

PersonResultList = PersonResultList
        .Where(pr => PersonList
                        .Any(p => pr.PersonId == p.PersonId
                              && p.Gender == "female"));

This is based on the assumption that for each entry in the first list there will be only one entry in the second list.


Encapsulate common parts of Entity Framework queries I'm developing a system with some complex logic regarding user permissions to access different data from a SQL Server database using C# and Entity Framework. I have different places across the system where I need to query certain DB entities called 'documents' that are only accessible to current user. I came up with the following class that queries only the necessary documents:

internal class DocumentPermissionsHelper
{
    private readonly MyDbContext dc;
    public DocumentPermissionsHelper(MyDbContext dc) { this.dc = dc; }

    public IQueryable<Document> SelectDocumentsAccessibleToUser(IQueryable<Document> query, Guid userId)
    {
        var permissionsQuery = dc. ... // really big query with multiple JOIN's to determine which user has access to which documents
        IQueriable<Guid> docIdsAccessibleToUserQuery =
            from permissions in permissionsQuery.Where(_ => _.UserId == userId && _.AccessLevel == "Read")
            select permissions.DocumentId;

        return from accessibleDocId in docIdsAccessibleToUserQuery.Distinct()
                from accessibleDoc in query.Where(g => g.Id == accessibleDocId)
                select accessibleDoc;
    }
}

Then I use it in several different queries like this:

public void GetLatestDocsCount(Guid currentUserId)
{
    using (var dc = new MyDbContext(_environment))
    {
        var helper = new DocumentPermissionsHelper(dc);
        var docQuery = dc.Docs.Where(d => d.CreatedDate > DateTime.Now.AddDays(-1));
        docQuery = helper.SelectDocumentsAccessibleToUser(docQuery, currentUserId);
        int latestDocsCount = docQuery.Count();
        return latestDocsCount;
    }
}

As I said, I have many different queries that produce different kinds of data, but all have one common part: the building of subquery that selects only documents accessible to a certain UserID.

Is there a better way to 'inject' this common query part into multiple Entity Framework queries rather than instantiate DocumentPermissionsHelper every time? My own solution seems somewhat clumsy to me.

Probably more can be made to clean that code but from your snippet I'd first introduce an extension method. You're using an object just to hold some parameters.

static class MyContextExtensions
{
    public static IQueryable<Document> SelectDocumentsAccessibleToUser(this MyDbContext dc, IQueryable<Document> query, Guid userId)
    {
        var permissionsQuery = dc. ... // really big query with multiple JOIN's to determine which user has access to which documents
        IQueriable<Guid> docIdsAccessibleToUserQuery =
            from permissions in permissionsQuery.Where(_ => _.UserId == userId && _.AccessLevel == "Read")
            select permissions.DocumentId;

        return from accessibleDocId in docIdsAccessibleToUserQuery.Distinct()
                from accessibleDoc in query.Where(g => g.Id == accessibleDocId)
                select accessibleDoc;
    }
}

Simply used like this:

public void GetLatestDocsCount(Guid currentUserId)
{
    using (var dc = new MyDbContext(_environment))
    {
        var docQuery = dc.Docs.Where(d => d.CreatedDate > DateTime.Now.AddDays(-1));
        docQuery = dc.SelectDocumentsAccessibleToUser(docQuery, currentUserId);
        int latestDocsCount = docQuery.Count();
        return latestDocsCount;
    }
}

Few other things:

You named a lambda parameter as _ and you're actually using it. It's not a rule but as widely accepted convention _ is reserved for an unused parameter. Rename it to something appropriate (or...x, at least).
Do not hard-code "Read" string, you might want to introduce a private const string field for that. From your snippet it may be local to MyContextExtensions but I guess you use it also elsewhere. Is there a reason you're using a string for that? It's a good candidate to be an enum.
I feel strange when I see mixing plain LINQ calls and query syntax. If there is no other reason you may want to stick to one of them (at least within same expression).

For example:

IQueriable<Guid> docIdsAccessibleToUserQuery = permissionsQuery
    .Where(_ => _.UserId == userId && _.AccessLevel == "Read")
    .Select(x => x.DocumentId);

latestDocsCount local variable does not really help to understand better your code, you may drop it and simply return docQuery.Count();.
It's opinable but IMO if you keep functions short (good!) then docQuery might simply be query.
DateTime.Now.AddDays(-1) shouldn't be hard-coded, it's an important peace of information. I'd move that query into a separate method GetDocumentsCreatedYesterday().
You're using DateTime.Now instead of DateTime.UtcNow. It may be or not an issue in your case but consider that time is adjusted (summer time) then twice per year you may get incorrect results. Also remember that if your users are spread all over the world then you'd better to store UTC time and format to local time only for presentation.
After our first refactoring step you do not have DocumentPermissionsHelper class but in general don't forget to mark a class as sealed when it's not intended to be extended.


Determining common divisors of two numbers Is there a better algorithm for finding the common divisors of two numbers?  Can this code be shortened?

import java.util.*;

public class Testing1 {
public static void main (String args[]){
    Scanner x=new Scanner(System.in);
    System.out.print("Enter the 1st number(larger number) :  ");
    int y=x.nextInt();
    System.out.print("Enter the 2nd number : ");
    int z=x.nextInt();

    int ar1[]=new int[y];
    int ar2[]=new int[z];

    for( int i=1;i<y;i++){ //puts the divisors of the larger number to an array ar1[]
         int l=y%i;
         if(l==0)
           ar1[i]=i;}

    for( int i=1;i<z;i++){//puts the divisors of the other number to an array ar2[]
        int l=z%i;
        if(l==0)
          ar2[i]=i;}System.out.print("Common divisors of "+y+", "+z+" = ");

    for(int i=1;i<=ar2.length-1;i++){ //printing the common elements of both arrays.
     if((ar1[i]==ar2[i])&&ar2[i]!=0)
        System.out.print(i+" ");
}}}

There are few improvements you can do in your code:

First choose better variable names. I would name the Scanner type variable as scanner rather than x. Really, x, y, z are the worst variable name, which doesn't give any idea as to what they denote.
Secondly, I won't force user to pass larger number first, and then smaller number. I would take the burden of finding that out myself.
There is no need of storing all the divisors of both the numbers in two different arrays. Suppose you have to find common divisor of 2 and 2132340. Will you store all the divisors of 2132340, or simply check that the common divisor cannot be greater than the divisor of 2, and use that fact?
Another fact is, all the divisors of num1 will be less than or equal to num1 / 2. Now, if num1 < num2, then the common divisors all must be < num1 / 2. So, iterate the loop till min / 2.
Of course, you can move the code that finds the common divisor in a different method, and store them in a List<Integer>.
Lastly, it might be possible that the smaller number itself is a divisor of larger number. So, you need to store that too.

Here's the modified code which looks a bit cleaner:

public static void main (String args[]){
    Scanner scanner = new Scanner(System.in);

    System.out.print("Enter the 1st number :  ");
    int num1 = scanner.nextInt();
    System.out.print("Enter the 2nd number : ");
    int num2 = scanner.nextInt();

    List<Integer> commonDivisor = getCommonDivisor(num1, num2);
    System.out.println(commonDivisor);
}

public static List<Integer> getCommonDivisor(int num1, int num2) {

    List<Integer> list = new ArrayList<Integer>();

    int min = minimum(num1, num2);

    for(int i = 1; i <= min / 2; i++) {
        if (num1 % i == 0 && num2 % i == 0) {
            list.add(i);
        }
    }

    if (num1 % min == 0 && num2 % min == 0) {
        list.add(min);
    }

    return list;
}

public static int minimum(int num1, int num2) {
    return num1 <= num2 ? num1 : num2;
}


A faster implementation of AcceptChanges? So I started wondering why it takes 3.5 seconds to AcceptChanges for one changed record in a large DataTable. I wrote another implementation of AcceptChanges (VB.NET) - Me is a DataTable here:

Public Shadows Sub AcceptChanges()
  'this way AcceptChanges is 35 to 700 faster than Microsoft's way
  '35 is a speed factor when all rows were changed, and 700 is when a few
  'Tested on sets of 13K and 65K records
  Dim updatedRows() As DataRow = Me.Select(Nothing, Nothing, _
    DataViewRowState.Added Or DataViewRowState.Deleted Or _
    DataViewRowState.ModifiedCurrent Or DataViewRowState.ModifiedOriginal)
  For Each row As DataRow In updatedRows
    row.AcceptChanges()
  Next
End Sub

It appears to be from 35 to 700 times faster than Microsoft's implementation. Did I just uncover/solve a great mystery of ADO.NET, or am I missing something here?

It's quite easy to answer your question by looking at the DataTable.AcceptChanges source code.

As you can see they copy all DataRow objects into new array before iterating them. I suspect that your code will work incorrectly when rows are deleted (accepting changes for deleted rows means removing them from DataTable), most likely you'll get exception since it's not allowed to change collection while iterating it.


Handling signals in Python inside a function I have code that needs to exit gracefully. To simplify things, I am presenting an example based on the answer given here.

Since I need to handle a few signals, I thought of putting this logic in a function:

def set_signals():
   original_sigint = signal.getsignal(signal.SIGINT)
   signal.signal(signal.SIGINT, exit_gracefully)
   signal.signal(signal.SIGTERM, exit_gracefully)
   signal.signal(signal.SIGINT, exit_gracefully)
   signal.signal(signal.SIGALRM, exit_gracefully)
   signal.signal(signal.SIGHUP, signal.SIG_IGN)

Thus, the main block of the python code should be:

if __name__ == '__main__':
    # store the original SIGINT handler
    set_signals()
    run_program()

This will fail however, since the exit_gracefully does not know the variable original_init.

Therefore, my solution was to create original_sigint as a global variable.

import signal
import time
import sys

original_sigint = None

def run_program():
    while True:
        time.sleep(1)
        print("a")

def exit_gracefully(signum, frame):
    # restore the original signal handler as otherwise evil things will happen
    # in raw_input when CTRL+C is pressed, and our signal handler is not re-entrant
    global original_sigint
    signal.signal(signal.SIGINT, original_sigint)

    try:
        if raw_input("\nReally quit? (y/n)> ").lower().startswith('y'):
            sys.exit(1)

    except KeyboardInterrupt:
        print("Ok ok, quitting")
        sys.exit(1)

    # restore the exit gracefully handler here
    signal.signal(signal.SIGINT, exit_gracefully)

def set_signals():
    global original_sigint
    original_sigint = signal.getsignal(signal.SIGINT)
    signal.signal(signal.SIGINT, exit_gracefully)
    signal.signal(signal.SIGTERM, exit_gracefully)
    signal.signal(signal.SIGINT, exit_gracefully)
    signal.signal(signal.SIGALRM, exit_gracefully)
    signal.signal(signal.SIGHUP, signal.SIG_IGN)

if __name__ == '__main__':
    # store the original SIGINT handler
    set_signals()
    run_program()

This approach works, but I am not happy with the use of another global variable (the code I working on is quite and is already littered with those). So the question is, how would you do it differently?

What about implementing your signal handling code inside a class? This could look something like the following:

class GracefulExit:
    def __enter__(self):
        # set up signals here
        # store old signal handlers as instance variables

    def __exit__(self, type, value, traceback):
        # restore old signal handlers

You can then use this in your code as follows:

with GracefulExit():
    # Signals will be caught inside this block.

# Signals will no more be caught here.

You'll find more examples of how to use the with-statement on the web.