Distribute stats randomly I have returned to programming after a year without practice and this is also my first time using Java. I know there is a more efficient way to do this so any suggestions would be appreciated.

I am trying to distribute stats randomly to a character in my game. However, I can't just go through each stat and increase/decrease each one randomly. I need the total increases to be equal to the total decreases. So if one stat goes up 5%, another will need to decrease 5% to compensate.

Egg make_egg(){

float newgr = growth_rate;
float newms = maxSpeed;
int newsf = starve_frames;
int newls = lifespan;

ArrayList<Integer> stats = new ArrayList<Integer>();
stats.add(0);
stats.add(1);
stats.add(2);
stats.add(3);

float percentage = 0;
int roll_stat = 0;
while(stats.size() > 1){
  percentage = random(0,1);
  //get the value of a random element in the arraylist
  roll_stat = stats.get(round(random(0, stats.size()-1)));
  //remove(int) is ambiguous for an ArrayList<Integer>
  //am I doing this right?
  stats.remove(Integer.valueOf(roll_stat));

  //--------------LOSSES-------------//
  if(roll_stat == 0){           //gr
      newgr -= percentage*growth_rate_delta;
      if(newgr <= minimum_growth_rate){
        newgr = minimum_growth_rate;
      }
  }
  else if(roll_stat == 1){      //ms
      newms -= percentage*maxspeed_delta;
      if(newms <= minimum_maxspeed){
        newms = minimum_maxspeed;
      }
  }
  else if(roll_stat == 2){      //sf
     newsf -= round(percentage*starve_frames_delta);
     if(newsf <= starve_frames_minimum){
        newsf = starve_frames_minimum;
      }
  }
  else if(roll_stat == 3){      //ls
     newls -= round(percentage*lifespan_delta);
     if(newls <= lifespan_minimum){
        newls = lifespan_minimum;
      }
  }

  roll_stat = stats.get(round(random(0, stats.size()-1)));
  stats.remove(Integer.valueOf(roll_stat));

  //--------------GAINS-------------//
  if(roll_stat == 0){            //gr
      newgr += percentage*growth_rate_delta;
      if(newgr >= maximum_growth_rate){
        newgr = maximum_growth_rate;
      }
  }
  else if(roll_stat == 1){      //ms
      newms += percentage*maxspeed_delta;
      if(newms >= maximum_maxspeed){
        newms = maximum_maxspeed;
      }
  }
  else if(roll_stat == 2){      //sf
     newsf += round(percentage*starve_frames);
     if(newsf >= starve_frames_maximum){
        newsf = starve_frames_maximum;
      }
  }
  else if(roll_stat == 3){      //ls
     newls += round(percentage*lifespan_delta);
     if(newls >= lifespan_maximum){
        newls = lifespan_maximum;
      }
  }

}

return new Egg(x, y, newgr, newms, newsf, newls, generation + 1);
}

I know this will not work if there are an odd number of stats, but I can deal with that later. One other problem this has is that two stats will always decrease and two stats will always increase. However, the ideal effect would be if, for example, one stat could increase by 9%, and the other three stats could decrease by 5, 3, and 1% each.

Reviewing the code as it is, rather than the problem with the statistical distribution.

Indentation

Your indentation is off and thus makes the code hard to read. Everything inside the method should be indented. Also it seems like some of the indentations are differently deep. Indentations in Java are usually 4 spaces.

Access modifiers

Your method does not have an access modifier (at least not explicitly), so I assume you forgot to add one. Access modifiers are important to control from where in the code the method (or class or member etc.) can be acccessed. Since your method does not have one, it implicitly has the default modifier, which is package-private. This might be correct, but if you just forgot it, use private if the method is only called within the same class.

Naming

Your variable names are unclear. In the context of float newgr = growth_rate; one might guess that newgr may mean something like "new growth rate", but without context it is completely unclear what it means. Use longer and more explicity names.

Also the names on the right part of the assignment are unconventional, because in Java you usually want to use camelCase for variables and methods, and UpperCamelCase aka PascalCase for classes and interfaces.

Your variable declarations should probably be something like this:

float newGrowthRate = growthRate;
float newMaxSpeed = maxSpeed;
int newstarveFrames = starveFrames;
int newLifeSpan = lifeSpan;

The same applies to some other variable names further down your code.

Your percentage variable is not really a percentage, because it can have values between 0 and 1. Percentages are values between 0 and 100, so you could change the scaling and use a random calue between 0 and 100, or simply rename the variable to something like factor or scaling. If you don't like any of those names and can't come up with a better one, change it to normalizedPercentage.

Style

In addition to the indentation, you are using newlines and whitespace in an unusual way.

For example

while(stats.size() > 1){

should be written like

while (stats.size() > 1) {

This makes the code more readible and consistent. Also around operators there should be spaces, like percentage * maxspeed_delta instead of percentage*maxspeed_delta.

Switch-Case

Using the switch-syntax you can make your code more readible where you are using many if-else blocks with integer comparison conditions.

So this

if(roll_stat == 0){           //gr
    newgr -= percentage*growth_rate_delta;
    if(newgr <= minimum_growth_rate){
        newgr = minimum_growth_rate;
    }
}
else if(roll_stat == 1){      //ms
    newms -= percentage*maxspeed_delta;
    if (newms <= minimum_maxspeed) {
        newms = minimum_maxspeed;
    }
}

can become this

switch (roll_stat) {
    case 0:
        newgr -= percentage * growth_rate_delta;
        if(newgr <= minimum_growth_rate){
            newgr = minimum_growth_rate;
        }
        break;
    case 1:
        newms -= percentage * maxspeed_delta;
        if (newms <= minimum_maxspeed) {
            newms = minimum_maxspeed;
        }
        break;
}

Don't forget the break after every case. If you have an else at the end (instead of an else if) use the default: case.

Comments

//get the value of a random element in the arraylist
roll_stat = stats.get(round(random(0, stats.size()-1)));

This comment is obsolete, because it just says what the code in the next line says anyway. Instead try to write self-documenting code, for example by selecting the random index first and saving it in a variable, so that the variable's name tells you what it is. Also you can directly get a random integer value, so you don't have to round. Then you get

Random random = new Random();
// nextInt(n) takes the upperbound exclusively, thus not stats.size() - 1
int randomIndex = random.nextInt(stats.size());
randomStat = stats.get(randomIndex);

Comments should tell why something is done, not what is done. What is done should be told by the code.

You are using //--------------LOSSES-------------// and //--------------GAINS-------------// to separate logical parts of your code. It is better to move these parts into methods and have the methods name explain what is done.

Removing items from ArrayList<Integer>

In a comment you ask whether you do this correctly. If I understand your intent correctly, then you are not doing it right, and it only makes no difference in your case because your items' values are the same as their indexes.

It is important to note that int and Integer are not the same thing. They are two different types, Integer being a wrapper for the primitive int. In some cases the JVM automatically "boxes" these ints into Integer objects or "unboxes" them the other way around. Since an ArrayList<Integer> contains Integer objects, when doing stats.remove(Integer.valueOf(roll_stat)); the ArrayList will search for the contained object if it exists in the list, and stats.remove(roll_stat); will remove the item at the index roll_stat. Since you randomly select an item by calculating a random index, you might as well save the index in a variable (see my example above where I do it for better readability anyway) and then remove by index.

Initializing your ArrayList

This seems a bit verbose:

ArrayList<Integer> stats = new ArrayList<Integer>();
stats.add(0);
stats.add(1);
stats.add(2);
stats.add(3);

Instead you can do this:

ArrayList<Integer> stats = new ArrayList<>(Arrays.asList(0, 1, 2, 3));


Dynamically initializing Context and creating connection string for Web API Scenario:
There is a 20 years old windows application with SQL Server as database. There are 42 companies within the Group and each company has its own database.
Now we have started building new applications and all critical data like finances etc need to come from and be written back to old system's database.

To make this easier and uniform across all new applications I've started an API project. Issue obviously is to map to specific database based on company from old databases while also use Entity framework for new databases.

My solution is to use mix of Entity Framework and Dapper.

If there are complex queries required or stored procedures are already present, then we use Dapper else for simple APIs for new databases we use Entity Framework.

I have used Application_BeginRequest event of global.asax to initialize database. I'm passing company code in headers to identify which database to use and form a connection string.

I'm also using Mehdime.Entity to manage transaction scopes and DbContexts.

here is the Application_BeginRequest code

protected void Application_BeginRequest(Object source, EventArgs e)
    {
        var request = Request;
        var requestHeaders = request.Headers.Keys;

        var companyCode = request.Headers.GetValues("CompanyCode");
        var builder = new ContainerBuilder();

        ApiVariables.Company = companyCode?[0];
        var connectionString = ApiVariables.GetConnectionString(ApiVariables.Company);
        // Get your HttpConfiguration.
        var config = GlobalConfiguration.Configuration;

        builder.RegisterGeneric(typeof(ApiRepository<>)).As(typeof(IRepository<>));
        builder.RegisterAssemblyTypes(Assembly.Load("HRQ.Service"))
            .Where(t => t.Name.EndsWith("Service")).AsImplementedInterfaces().InstancePerLifetimeScope();

        builder.RegisterType<DbContextScopeFactory>().As<IDbContextScopeFactory>();
        builder.RegisterType<AmbientDbContextLocator>().As<IAmbientDbContextLocator>();
        builder.RegisterType<ApiDbContext>().As<IApiDbContext>().WithParameter("connectionString", connectionString);

        // Register your Web API controllers.
        builder.RegisterApiControllers(Assembly.GetExecutingAssembly());

        // OPTIONAL: Register the Autofac filter provider.
        builder.RegisterWebApiFilterProvider(config);

        // Set the dependency resolver to be Autofac.
        var container = builder.Build();
        config.DependencyResolver = new AutofacWebApiDependencyResolver(container);

        Database.SetInitializer<ApiDbContext>(new CreateDatabaseIfNotExists<ApiDbContext>());
    }

For queries to old database, I use:

using (var connection = DapperConnectionFactory.GetOpenConnection())
        { //execute queries here}

To initiate connection for dapper:

public static DbConnection GetOpenConnection()
    {
        var connection = new SqlConnection(ApiVariables.GetConnectionString(ApiVariables.Company));
        connection.Open();
        return connection;
    }

Am I in right path? or is this a terrible code?

I am not familiar with Dapper, but my assumption is that the objectives of using it are similar to using any other data access framework. Just from looking at their GitHub page, it looks like a very nice tool.

Unfortunately, maintaining compatibility with old applications is the reality that enterprises have to deal with, so in concept, I don't think this is a terrible idea. I would suggest doing something to ensure you have good connectivity between your API and your databases, though. If your company uses QOS, and the MSSQL traffic is not prioritized, but HTTP is, then using a SOAP service at the data source might be a good idea, and then just forward the request to that service. Web API would also be good, but implementation is a little more complex. That may be okay, though, if you want to avoid the overhead of SOAP envelopes.

From an API perspective, I would not recommend using custom HTTP headers. Part of the goal of Web API is having a standards compliant HTTP endpoint. Although that does not exclude using custom headers, I think that it might be better to try and device a custom route for the legacy integration, in places where you do not need to integrate with the modern systems. An alternative approach, if the intent is to store the data twice, is to dependency inject both your EF context, and your legacy adapter, into the controller. When the legacy adapter can go away, you "just" remove it from your controller. That way, you can integrate it across all of your API, and provide as seamless an experience as possible for your consumers.

Good luck with your implementation. I'm working on resolving several legacy applications myself, and it seems to be tough work whenever I hear anyone mention it.


Reversing a doubly linked list in C Looking for feedback on this code to reverse a double linked list. Things I am potentially looking for but missed -> redundant operations, extra / unnecessary arithmetic, improvement in code style. Thank yoU!

typedef struct Node {
    int data;
    struct Node* next;
    struct Node* prev;
} Node;

void reverse(Node** head) {

    Node* curr = *head;
    Node* prev_ptr = NULL;
    Node* next_ptr = NULL;
    while (curr != NULL) {
        prev_ptr = curr->prev;
        next_ptr = curr->next;
        curr->prev = next_ptr;
        curr->next = prev_ptr;
        (*head) = curr;
        curr = next_ptr;

    }

}

You don't need to reverse a doubly linked list, which is an O(n) operation.
Just define a structure that will hold the head and tail of the list.
struct List
{
  Node* head;
  Node* tail;
  unsigned int count; // you can also keep track of number of nodes to access the count in O(1)
}

Functions manipulating the list shall now accept List* list rather then Node** head. They will also have to contain logic that checks and assigns the tail, but none of the operations should get any more complex in terms of its big-O time complexity.
Now traversing the list in reversed order is just matter of traversing the list from tail to head, rather then head to tail direction.
Doubly linked list without a tail is basically a singly linked list with capability to traverse back from where you already traversed forward, but never directly from the tail towards the head (without additional effort).


Test many sorting algorithms using Python and Nose I'm trying to learn how to build a good test suite, using simple sorting as an example.
Particularly, I am using Nose to generate all the tests based on a simple list of lists, where each sublist is a unit-test for each sorting method, as seen below:

from nose.tools import assert_equal

def check_sort(s, arr):
    assert_equal( s(arr), sorted(arr) )

def selection_sort(arr):
    """Repeatedly find the smallest element in the unsorted part
    and swap it with the first element in the unsorted part"""
    for i in xrange(len(arr)):
        min_val = min(arr[i:])
        min_ind = i + arr[i:].index(min_val)
        arr[i], arr[min_ind] = arr[min_ind], arr[i]
    return arr

def test_sorts():
    sorts = [selection_sort]
    for s in sorts:
        examples = [ [],\
                 [1],\
                 [1,1],\
                 [1, 2, 3],\
                 [1, 3, 2],\
                 [2, 1, 3],\
                 [2, 3, 1],\
                 [3, 1, 2],\
                 [3, 2, 1],\
                 [-10, 35, 4, 99, 1003, 2],\
                 [-10, 2, 35, 4, 4, 99, 99, 1003, 2]]
        for arr in examples:
            yield check_sort, s, arr

It seems to me that there has to be a better way of doing this, by using classes and more advanced methods to set-up and tear-down on the go. The caveat is that I would like to be able to extend the sorts list, adding more functions that implement bubble sort, merge sort, quick sort, etc. without having to create more tests by hand.

Any advice is appreciated.

You don't need backslashes for line continuation inside any pair of brackets. Remove them. Further, this formatting IMHO is quite strange anyway.

The second problem is that you are both modifying the sorted array and returning it. Note how list.sort modifies but doesn't return and sorted returns but doesn't modify. Considering how slow your sort is, a copy at the start wouldn't hurt.

Your docstring is also all wrong; docstrings should mention user-facing concerns, not implementation detail. A thorough one might say

Sort the input container in place and return it.

This sorts in n² time and is not guaranteed to be stable.
The input must be mutable and its elements must have total order.

Talking about the tests, I'm going to ignore the framework and focus on the meat of the problem. I would try and get automatic test generation down first.

You really want two things. First, for small values do an exhaustive check:

from itertools import product

def test_sort_small(sort):
    for length in range(8):
        for sortable in product(range(4), repeat=length):
            sortable_copy = list(sortable)
            sorted_copy = sort(sortable_copy)

            assert sorted_copy is sortable_copy
            assert sorted_copy == sorted(sortable)

For large values do a random check.

import random

def test_sort_large(sort):
    for log_length in range(4, 12):
        for length in range(2**log_length-4, 2**log_length+5):
            sortable = [random.randint(0, 3) for _ in range(length)]

            sortable_copy = list(sortable)
            sorted_copy = sort(sortable_copy)

            assert sorted_copy is sortable_copy
            assert sorted_copy == sorted(sortable)

Tie those together

def test_sorts(sorts={selection_sort}):
    for sort in sorts:
        test_sort_small(sort)
        test_sort_large(sort)

test_sorts()

Put this logic in whatever framework you want.

Personally I'd also throw in a test with a more compex object, but it shouldn't really matter.

I would definitely try testing with another container, such as a bytearray, if you want to support them.

You could easily get a stable sort, so you might want to add that and add associated tests.


Testing Video Frames for Uniqueness I'm working on a program that will (hopefully) eventually allow me to perform frame rate analysis like Digital Foundry does.  The code below works (as far as I know), but before I iterate on it, I'd like some feedback as I'm new to both C++ and OpenCV.  I say "as far as I know" because I'm not sure how "lossless" my lossless capture is or of the correctness of the program.  Is this code a good start?

#include "opencv2\opencv.hpp"
#include <fstream>

using namespace cv;
using namespace std;

int main(int argv, char** argc) {
    Mat curFrame;
    Mat pastFrame;
    VideoCapture vid("E:\\videos\\video.mkv");
    ofstream textFile("diffs.txt");
    int numDiff = 0;
    int numFrames = 0;

    if (!textFile.is_open()) {
        return -1;
    }

    if (!vid.isOpened()) {
        return -2;
    }

    vid.read(pastFrame);
    while (vid.read(curFrame)) {
        for (int i = 0; i < curFrame.rows; i += 1) {
            cv::Vec3b* curPixel = curFrame.ptr<cv::Vec3b>(i);
            cv::Vec3b* pastPixel = pastFrame.ptr<cv::Vec3b>(i);
            for (int j = 0; j < curFrame.cols; j += 1) {
                bool found = false;
                if (curPixel[j][0] != pastPixel[j][0]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][1] != pastPixel[j][1]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][2] != pastPixel[j][2]) {
                    numDiff += 1;
                }
            }
        }
        numFrames += 1;
        //cout << "Frame " << numFrames << endl;
        double size = curFrame.rows * curFrame.cols;
        textFile << numFrames << "," << fixed << setprecision(2) << numDiff / size * 100 << "\n";
        pastFrame = curFrame.clone();
        numDiff = 0;
    }

    textFile.close();
    vid.release();
    return 0;
}

Its pretty good:

Don't do this:

using namespace cv;
using namespace std;

Its a bad habit that one day will get you into a lot of trouble. The reason the namespace names are so short is so that adding std:: or cv:: before identifiers is not overburdensome.

Read more about the issues: Why is “using namespace std” considered bad practice?

I suppose this is valid:

int main(int argv, char** argc) {

But most people use this:

int main(int argc, char* argv[]) {
                         ^^^^   => argv: argument value
                       ^     ^^ => Easier to see its an array of pointers rather **
             ^^^^               => argc: argument count

Declare variables at point of usage.

In C++ we don't tend to declare all the variables at the top of the function. It is much nicer to declare them just before you use them. Remember all objects have constructors that are run so declaring it before you need it may cost you in execution if you don't need it.

But the main reason is that types are important. If you declare it at the point you use it then it becomes easy to see the type and thus reason about it. If I have to scroll back to the top of the function it makes it hard to keep track of thing.

Streams can detect when they have gone bad.

Not opening the file is not the only type of error. So only testing for that seems like a short cut that may backfire.

    if (!textFile.is_open()) {
        return -1;
    }

I would use:

    if (!textFile) {    // If a std::fstream is used in a boolean context
        return -1;      // it converts itself to bool be checking is_good()
    }                   // which will fail if you can't read from it in any
                        // way.

Not sure if that is true for VideoCapture type (have a look). But given the read works like a std::stream (see the while loop) I am suspecting it does.

    if (!vid.isOpened()) {
        return -2;
    }

Prefer pre-increment

        for (int i = 0; i < curFrame.rows; i += 1) {
            for (int j = 0; j < curFrame.cols; j += 1) {

Sure that works. But prefer ++i (or ++j). It is more idiomatic. Also when you start incrementing using iterators the pre-increment is the cheapest increment.

Don't need found

                bool found = false;
                if (curPixel[j][0] != pastPixel[j][0]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][1] != pastPixel[j][1]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][2] != pastPixel[j][2]) {
                    numDiff += 1;
                }

Each of those pieces of code are completely unique. Because you use else only one of those branches can be executed.

I would simplify and write as:

            numDiff += ((curPixel[j][0] != pastPixel[j][0]) ||
                        (curPixel[j][1] != pastPixel[j][1]) ||
                        (curPixel[j][2] != pastPixel[j][2]))  ? 1 : 0;

Based on Toby's comments this can be simplified too:

            numDiff += curPixel[j] != pastPixel[j] ? 1 : 0;

Cloning is probably expensive

        pastFrame = curFrame.clone();

Have your tried to see if a move operation exists?

        pastFrame = std::move(curFrame);

Don't manually close files

    textFile.close();

Unless you are checking to see if the close worked and generating an error message when it fails. Otherwise just let the constructor call close and throw away any potential exceptions that are generated.

On the same note. Do you really need to manually release the vid?

    vid.release();

Version 2

#include "opencv2\opencv.hpp"
#include <fstream>

int main(int argc, char* argv[])
{
    std::ofstream textFile("diffs.txt");
    if (!textFile.is_open()) {
        return -1;
    }

    cv::VideoCapture vid("E:\\videos\\video.mkv");
    if (!vid.isOpened()) {
        return -2;
    }

    int numFrames = 0;

    cv::Mat pastFrame;
    vid.read(pastFrame);

    cv::Mat curFrame;
    while (vid.read(curFrame)) {
        int numDiff = 0;
        for (int i = 0; i < curFrame.rows; ++j) {

            cv::Vec3b* curPixel = curFrame.ptr<cv::Vec3b>(i);
            cv::Vec3b* pastPixel = pastFrame.ptr<cv::Vec3b>(i);

            for (int j = 0; j < curFrame.cols; ++j) {
                numDiff += curPixel[j] != pastPixel[j] ? 1 : 0;
            }
        }
        numFrames += 1;
        double size = curFrame.rows * curFrame.cols;
        textFile << numFrames << ","
                 << fixed
                 << setprecision(2) << numDiff / size * 100
                 << "\n";
        pastFrame = curFrame.clone();
    }
}


Naïve RSA decryption in Python I am making a code with basic RSA encryption/decryption.  My professor wants me to speed up this function but it is already so simple and I am lost.  Any ideas?

def decrypt(kenc,d,n):
    kdec=(kenc**d)%n
    return kdec

Simple does not mean fast, so you cannot judge performance based on how simple the implementation looks. Usually the most efficient way to perform a non-trivial task is not also the simplest way to do it. In this case though, there is a much more efficient solution that is about equally simple, and is probably sufficient.

There is a serious problem with this implementation: it computes kenc**d.

kenc**d is in general a very big number that takes a long time to compute, and then it takes a long time again to reduce it modulo n. For example, trying it out with 1024bit RSA (the lowest setting!):

import Crypto
from Crypto.PublicKey import RSA
from Crypto import Random

random_generator = Random.new().read
key = RSA.generate(1024, random_generator)

def decrypt(kenc,d,n):
  kdec=(kenc**d)%n
  return kdec

(ciphertext,) = key.encrypt(42, 0)
print(decrypt(ciphertext, key.d, key.n))

This does not finish in a reasonable time.

There is a simple remedy: use modular exponentiation, which keeps the size of the numbers that it is working with low throughout the whole calculation by reducing modulo n as it goes along. You could implement it yourself, but Python handily provides a built-in function for this: pow(x, e, n)

So decrypt can be written as:

def decrypt(kenc, d, n):
  return pow(kenc, d, n)

With that change, the code above decodes the message quickly.

Further improvements are possible, but more complicated, and won't be drop-in replacements.


Pythonic way to find duplicate objects in a set I have tons of location points with several attributes like time_stamp, latitude, longitude, accuracy, velocity etc. Now I want to remove duplicate location points on these conditions:

Latitude is same
Longitude is same
Month is same
Day is same

The other attributes dont matter at all for comparison. My strategy is to modify __hash__, __eq__ and __ne__ methods to include the above conditions and feed them into set function to remove the duplicates.

item1 = Location(1510213074679, 286220203, 772454413, 1414, None, None, None, 78, None)
item2 = Location(1510213074679, 286220203, 772454413, 5, 6, 80, 226, None, None)
item3 = Location(1523620644975, 286265651, 772427842, 65, None, None, 193, 10, None)

x = set()
x.add(item1)
x.add(item2)
x.add(item3)
print(x)
{<__main__.Location at 0x7fd725559eb8>, <__main__.Location at 0x7fd725604dd8>}

Length of set is as expected i.e 2 (two elements with same attributes).

The result which I expect, can be obtained as follows

[f.data() for f in x]

Is there any other pythonic way to achieve the same result?

class Location(object):
    def __init__(self, time_stamp, latitude, longitude, accuracy, velocity,
                 heading, altitude, vertical_accuracy, activity):
        self.time_stamp = float(time_stamp) / 1000
        self.latitude = float(latitude) / 10000000
        self.longitude = float(longitude) / 10000000
        self.accuracy = accuracy
        self.velocity = velocity
        self.heading = heading
        self.altitude = altitude
        self.vertical_accuracy = vertical_accuracy
        self.activity = activity

        self.timestamp, self.year, self.month = month_aware_time_stamp(
            self.time_stamp)
        self.hash = self.hashed()

    def data(self):
        return self.__dict__

    def hashed(self):
        string = str(self.latitude) + str(self.longitude)
        return hashlib.sha3_224(string.encode()).hexdigest()

    def __hash__(self):
        string = str(self.latitude) + str(self.longitude)
        return hash(string.encode())

    def __eq__(self, other):
        """Override the default Equals behavior"""
        if isinstance(other, self.__class__):
            return self.hash == other.hash and self.month == other.month and self.year == other.year
        return False

    def __ne__(self, other):
        """Override the default Unequal behavior"""
        return self.hash != other.hash or self.month != other.month or self.year != other.year

hash

Since tuples are hashable if their elements are hashable, you can just do

def __hash__(self):
    return hash((self.latitude, self.longitude, self.year, self.month))

If you want to have locations that are near each other be set as the same, you might have to round the coordinates

repr

for debugging, adding a repr can be handy:

def __repr__(self):
    return (
        "Position("
        f"lat: {self.latitude}, "
        f"lon: {self.longitude}, "
        f"year: {self.year}, "
        f"month: {self.month}, "
        ")"
    )

Counting

To count, you can use a collections.Counter, and just feed it an iterable of Positions.

counter = Counter(locations)
most_visited = counter.most_common(<n>)

You will need one Counter per year/month, but this can be done with a defaultdict(Counter)


PageObject implementation (CoffeeScript, CasperJS) I'm using CasperJS and CoffeeScript for integration testing of a hobby project. Page Objects seem to be a useful abstraction. I had two major requirements:

Support for node.js style callbacks - mainly because tests can then be written using async.waterfall
Support extending (subclassing) to encapsulate common functionality, like a sidebar or a navbar

Here's the code, followed by a brief example of how I use it. It's a bit long, but that's about the shortest meaningful example I can give.

class PO # Base PO
  @casper: null

  # name: The name of the page object. Used to access it as PO[name], which is kind of like a forward declaration
  # selector: The page represented by this PO is considered to be ready for usage once this is visible
  # mixin: An object; properties of this will be mixed into the new instance
  constructor: (@name, @selector, mixin) ->
    if PO[@name] isnt undefined
      throw "Failed creating PO #{@name}: a PO instance or class method with this name already exists"
    PO[@name] = this
    @__defineGetter__ 'casper', -> PO.casper
    this[key] = val for key, val of mixin

  waitUntilVisible: (cb) -> @casper.waitUntilVisible @selector, cb

  # Switch to po, and call the callback cb with err and the po as parameters
  next: (cb, po, err=null) ->
    t0 = Date.now()
    msg = "Switching to PO #{po.name}"
    @casper.log msg, 'debug'
    po.waitUntilVisible =>
      @casper.log "#{msg}: done in #{Date.now() - t0}ms", 'debug'
      cb err, po
    po

new PO 'navbar', '',
  tabs: {}
  logout: (cb) ->
    @casper.click '#logout'
    @casper.waitFor (=> not @haveLogin()), (=> cb null, PO['login'])

  haveLogin: (cb) ->
    result = nick == @casper.evaluate -> $('#current-user-nick').text()
    cb? result
    result

  addTab: ({name, switchSelector, readySelector, po}) ->
    @tabs[name] = arguments[0]

  toTab: (name, cb) ->
    cb "Navbar tab #{name} not known", null unless name of @tabs
    tab = @tabs[name]
    @casper.click tab.switchSelector
    @casper.waitUntilVisible tab.readySelector, => cb null, PO[tab.po]

class PON extends PO # PO with navbar
  constructor: (name, selector, mixin) ->
    super name, selector, mixin
    @navbar = PO['navbar']

new PON 'login', '.login-form',
  # ...

module.exports = (casper) ->
  PO.casper = casper
  PO['login']

Do we need to attach a casper reference to the PO class?
By using the module pattern you inject the dependency anyway.
Here is the P0 class with a tiny refactoring:

module.exports = (casper) ->

  class PO
    constructor: (@name, @selector, mixin={}) ->
      if PO[@name]?
        throw new Error "Failed creating PO #{@name}: ..."
      PO[@name] = @
      @[key] ?= val for own key,val of mixin

    waitUntilVisible: (cb) ->
      casper.waitUntilVisible @selector, cb
      @

    next: (po, cb) ->
      t0 = Date.now()
      msg = "Switching to PO #{po.name}"
      casper.log msg, 'debug'
      po.waitUntilVisible (err) ->
        casper.log "#{msg}: done in #{Date.now() - t0}ms", 'debug'
        cb err, po

Then you would use it like this:

casper = require 'casper'
PO     = require('MyModuleName') casper

new PO 'navbar', '',
  tabs: {}
  logout: (cb) ->
    casper.click '#logout'
    casper.waitFor (=> not @haveLogin()), (err) -> cb err, PO.login

  haveLogin: (cb) ->
    result = nick is casper.evaluate -> ($ '#current-user-nick').text()
    cb? result
    result

  addTab: ({name, switchSelector, readySelector, po}) ->
    @tabs[name] = arguments[0]

  toTab: (name, cb) ->
    cb new Error "Navbar tab #{name} not known" unless name of @tabs
    tab = @tabs[name]
    casper.click tab.switchSelector
    casper.waitUntilVisible tab.readySelector, (err)-> cb err, PO[tab.po]

On extending the class you can skip the constructor (as long as it's the same like the one of the superclass) and attach the PO['navbar'] to the prototype.

class PON extends PO
  navbar: PO.navbar


Python script that make my .bash_aliases file MUCH more readable and clear My .bash_aliases file contains several aliases.
In the beginning, when I wanted to see the aliases, I print the .bash_aliases file,
The output wasn't readable at all, so I created a python script to make it better.
To call my python script, I use the alias print_aliases_manual
What is the most pythonic way to write the following script?
Here is my python script:

class bcolors:
    HEADER = '\033[95m'
    OKBLUE = '\033[94m'
    OKGREEN = '\033[92m'
    WARNING = '\033[93m'
    FAIL = '\033[91m'
    ENDC = '\033[0m'
    BOLD = '\033[1m'
    UNDERLINE = '\033[4m'

with open('/home/pi/.bash_aliases', 'r') as f:
    lines = f.readlines()

    for line in lines:
        if r'##' in line:
            print(f'{bcolors.HEADER}{bcolors.UNDERLINE}{bcolors.BOLD}{line}{bcolors.ENDC}')
        elif r'#' in line:
            print(f'{bcolors.WARNING}{line}{bcolors.ENDC}')
        elif r'alias' in line:
            drop_alias = line.replace('alias', '')
            new_line = drop_alias.replace('=', '   ')
            print(f'{bcolors.OKGREEN}{new_line}')
        else:
            print(f'{line}')

Here is my bash_aliases file:
### Aliases list divided by category:

## General commands:
alias cdd="cd /home/pi/Desktop/speedboat/"
alias dc="sudo docker-compose"
alias pm2="sudo pm2"

## Cellular related commands:
alias pc="ping -I wwan0 www.google.com"
alias qmi="sudo qmicli -d /dev/cdc-wdm0 -p"

## AWS relaeted commands:
alias dlogs="sudo docker logs -f iot_service"

## Git commands:
alias gp="sudo git pull"
alias gc="sudo git checkout"

## scripts to run when deleting speedboat directory is needed:

# copying files to Desktop
alias stodtop="sudo -E sh /home/pi/Desktop/speedboat/scripts/copy_files_to_desktop.sh"
# copying files from Desktop
alias sfromdtop="sudo -E sh /home/pi/Desktop/speedboat/scripts/copy_files_from_desktop_to_speedboat.sh"

## verifications test and updates scripts
# run speedboat verifications tests
alias sinsta="sudo sh /home/pi/Desktop/speedboat/scripts/installation_verification.sh"
# run local unittests
alias slocalt="sudo sh /home/pi/Desktop/speedboat/scripts/run_local_unittest.sh"
# pull os updates from git and execute
alias supdate="sudo sh /home/pi/Desktop/speedboat/scripts/updates-speedboat.sh"

## speedboat start and stop scripts (dockers and PM2)
alias startr="sudo sh /home/pi/Desktop/speedboat/scripts/start_speedboat.sh"

alias stopr="sudo sh /home/pi/Desktop/speedboat/scripts/stop_speedboat.sh"

## AWS related scripts:

alias secrl="sudo sh /home/pi/Desktop/speedboat/scripts/ecr-login.sh"

alias slog="sudo sh /home/pi/Desktop/speedboat/scripts/log_register.sh"

## Command to run speedboat aliases manual:
alias print_aliases_manual="python3 /home/pi/Desktop/speedboat/verification/test_verification_speedboat/colored_aliases_manual.py"

print
f-Strings are a great and convenient tool for including variables and calculations in strings. I'd say they're generally not needed when printing only variables. For example
print(f'{bcolors.WARNING}{line}{bcolors.ENDC}')

could become
print(bcolors.WARNING, line, bcolors.ENDC, sep='')

However I'm not sure which of the two is more pythonic, I personally prefer the latter for readability.
You could also consider pulling out formatting into seperate variables to increase readability.
HEADER_FORMAT = "".join((bcolors.HEADER, bcolors.UNDERLINE, bcolors.BOLD, "{}", bcolors.ENDC))

print(HEADER_FORMAT.format(line))

in vs. startswith
I'd recommend using str.startswith instead of str in str for checking prefixes.
if r'##' in line:

becomes
if line.startswith(r"##"):

This is less error-prone and conveys your intentions more explicitly, without the need to look at the source file. As there aren't any escape characters in your prefixes, you also don't need to make it a raw string. You can simply drop the r:
if line.startswith("##"):


Making EmployeeHistory ViewModel immutable as code analysis suggested I want to display on a view the EmployeeHistory view model sorted by elements.assignEffDateNext and I want to instantiate and populate the view model from the controller. The code analysis is saying that the IList<IDMSElementDate> EmployeePositionsOverTime should be read only.  I kind of understand why.

This is what I started with.

public class EmployeeHistory
{
    public int EmployeeID { get; set; }
    public string EmployeeName { get; set; }
    public IList<IDMSElementData> EmployeePositionsOverTime { get; set; }

}

public async Task<ActionResult> EmployeeHistory(int? employeeID)
    {...//this part retrieves the elements from an api...

        Employee emp = db.Employees.Find(employeeID);
        string employeeName = emp.EmployeeFullName;

        var sortedList = elements.Elements.OrderByDescending(e => e.assignEffDateNext).ToList();

        EmployeeHistory empHist = new EmployeeHistory
        {
            EmployeeID = (int)employeeID,
            EmployeeName = employeeName,
            EmployeePositionsOverTime = sortedList,
        };

        return View("EmployeeHistory", empHist);
    }

This is what I have changed it to. I added a constructor and changed the properties to be read-only so the type is immutable now and the analyzer is no longer giving me a warning. I was wondering whether this is better now and whether there is anything else about it that I should change?

 public class EmployeeHistory
{

    public EmployeeHistory(int employeeID, string employeeName, IList<IDMSElementData> employeePositionsOverTime)
    {
        EmployeeID = employeeID;
        EmployeeName = employeeName;
        EmployeePositionsOverTime = employeePositionsOverTime;
    }

    public int EmployeeID { get; }
    public string EmployeeName { get;  }
    public IList<IDMSElementData> EmployeePositionsOverTime { get;  }

}

public async Task<ActionResult> EmployeeHistory(int? employeeID)
        {...//this part retrieves the elements from an api...

            Employee emp = db.Employees.Find(employeeID);
            string employeeName = emp.EmployeeFullName;

            var sortedList = elements.Elements.OrderByDescending(e => e.assignEffDateNext).ToList();

            EmployeeHistory empHist = new EmployeeHistory((int)employeeID,employeeName,sortedList);

            return View("EmployeeHistory", empHist);
        }

Nulls: What happens when someone asks for an EmployeeHistory without passing in an employee ID? It's hard for me to believe that the API for retrieving history will behave well, must less the call to db.Employees.Find(), and certainly not the cast (int)employeeID.

I would recommend either immediately checking this and bailing out (with an ArgumentNullException or an empty view, or whatever you think is best), if you aren't already doing so. Or, you could the signature to EmployeeHistory(int employeeID) - since without an employee ID there is very little you can do.

Names: You have an EmployeeHistory object, with three properties all named Employee-something. This is a bit redundant. Perhaps just ID, Name, and PositionsOverTime? Or PositionsHeld?

Although that would imply that that EmployeeHistory.ID is the ID of the history... But that raises the question to me, why copy these properties from the Employee object at all? Why not just hold a reference to the actual Employee?

In fact, perhaps it would make more sense to add the history of positions held to the Employee object. That does make more sense: "This employee has this history", rather than the other way around.

As a side note, I'll grant that abbreviations like emp, e, and empHist are easy enough to parse in this case. However, my preference would still be for complete words when possible. Remember that code is read far more often than it is written, so if a few extra keystrokes now save a few seconds of parsing later, that's a net win.

Encapsulation: You may have satisfied the requirement of a read-only collection in the eyes of your code analyzer, but you have not done so in fact. While a consumer of this class would not have the ability to swap out the entire list for one of their own, the IList<T> interface is permissive enough that they may as well. Here's a demo.

If you want to make this history truly read-only, you'll need to avoid handing out a direct reference to the list. List.AsReadonly() is good for this.

Another thing you may want to encapsulate is the order of the list. You have decided* that the history should be sorted with the most recent assignEffDateNext first. If you want to guarantee that histories are always ordered this way, the logic to do so belongs in the history's constructor.

*As a side note, I personally would not sort this list on the back end at all. Suppose a user wants to view the list in chronological order? Or sorted by position title? Javascript is perfectly capable of sorting arrays of the size you're likely to have. This gives your users flexibility while keeping (an admittedly very small) load off the server.

Robustness: "Be conservative in what you send, be liberal in what you accept." It's generally nice when functions only ask for what they need. As it stands, an EmployeeHistory cannot be constructed with a Collection of positions - even though just a simple IEnumerable ought to be sufficient. Save yourself future calls to .ToList(), and open up this constructor to be more permissive.

This same principle, in my mind, suggests that you should be as specific as possible about what you're returning. Making positions available as an IReadOnlyList is good, because it allows your users to access positions by index. For the same reason, I would also be more specific about what you're returning from the view. It is an ActionResult, yes... But more specifically it's a ViewResult. This may not have a big impact on your front end, but it could help with testability or refactoring down the line.

Here's how this code might look when you're done:

public class EmployeeHistory
{
    public Employee Employee { get; }
    public IReadonlyList<IDMSElementData> PositionsHeld { get; }

    public EmployeeHistory(
        Employee employee,
        IEnumerable<IDMSElementData> positions)
    {
        Employee = employee;

        PositionsHeld = positions
            .OrderByDescending(position => position.assignEffDateNext)
            .ToList()
            .AsReadOnly();
    }
}

public async Task<ViewResult> EmployeeHistory(int employeeID)
{
    var positions = GetPositionsFromSomeAPICall(employeeID);

    var employee = db.Employees.Find(employeeID);

    var history = new EmployeeHistory(employee, positions);

    return View("EmployeeHistory", history);
}


Postgres query to extract a JSON sub-object from a JSON field I have a json field in a table that I want to select certain fields from into a new json field. So far I have this, which works, but I feel may not be optimal:

select to_json(new_obj) from (
    select big_json_field->'json_property_1' as json_property_1,
    big_json_field->'json_property_2' as json_property_2
    from table_with_big_json_field
) as new_obj

Is there a function or method I'm missing to optimize this?

I'd recommend (if it isn't already) converting to the type from json to jsonb and adding a GIN index. The documentation below lays it out pretty clear. Good luck!
 https://www.postgresql.org/docs/9.4/datatype-json.html

For my recommendation, reason for using jsonb as opposed to vanilla json is that regular json is stored as an exact copy in postgres and you cannot add an index to that column. Whereas jsonb has a smaller footprint and allows for indexing the specific column. I'd prefer to store it in a format that could be indexed to allow for queries to be more performant.


Hackerrank problem: Climbing the Leaderboard (Java) I am solving the following Hackerrank problem: Climbing the Leaderboard.
The problem statement:
Alice is playing an arcade game and wants to climb to the top of the leaderboard and wants to track her ranking. The game uses Dense Ranking, so its leaderboard works like this:

The player with the highest score is ranked number 1 on the leaderboard.
Players who have equal scores receive the same ranking number, and the next player(s) receive the immediately following ranking number.

For example, the four players on the leaderboard have high scores of 100, 90, 90, and 80. Those players will have ranks 1, 2, 2, and 3, respectively. If Alice's scores are 70, 80 and 105, her rankings after each game are 4th, 3rd and 1st.
Function Description
Complete the climbingLeaderboard function in the editor below. It should return an integer array where each element res[j] represents Alice's rank after the jth game.
climbingLeaderboard has the following parameter(s):
scores: an array of integers that represent leaderboard scores
alice: an array of integers that represent Alice's scores
Input Format
The first line contains an integer n, the number of players on the leaderboard.
The next line contains n space-separated integers scores[i], the leaderboard scores in decreasing order.
The next line contains an integer, m, denoting the number games Alice plays.
The last line contains m space-separated integers alice[j], the game scores.
Constraints

1 <= n <= 2 * 10^5
1 <= m <= 2 * 10^5
0 <= scores[i] <= 10^9 for 0 <= i < n
0 <= alice[j] <= 10^9 for 0 <= j < m

The existing leaderboard, scores, is in descending order.
Alice's scores, alice, are in ascending order.
Subtask
For 60% of the maximum score:

1 <= n <= 200
1 <= m <= 200

Output Format
Print m integers. The jth integer should indicate Alice's rank after playing the jth  game.

My solution is failing to pass 4 test cases due to timeout. The code is as follows:
 import java.util.*;

    public class Main {
        public static void main(String[] args) {

            Scanner scanner = new Scanner(System.in);

            int totalPlayers = scanner.nextInt();
            ArrayList<Integer> playerScores = new ArrayList<>();
            for(int i = 0; i < totalPlayers; i++) {
                playerScores.add(scanner.nextInt());
            }

            int aliceTotalScores = scanner.nextInt();
            ArrayList<Integer> aliceScores = new ArrayList<>();
            for(int i = 0; i < aliceTotalScores; i++) {
                aliceScores.add(scanner.nextInt());
            }

            List<Integer> finalScores = new ArrayList<>(playerScores);
            for(int i = 0; i < aliceTotalScores; i++) {
                finalScores.add(aliceScores.get(i));
                finalScores.sort(Collections.reverseOrder());
                Set<Integer> set = new LinkedHashSet<>(finalScores);
                finalScores.clear();
                finalScores.addAll(set);
                System.out.println(finalScores.indexOf(aliceScores.get(i)) + 1);
            }
        }
    }

Any suggestions on how to improve the solution or some general direction as to where I am making a mistake would be appreciated. Thanks in advance.

I have some suggestions to make the code cleaner, not faster.
Always try to pass the size of the maximum size to the Collection / Map when known
The ArrayList has a default size of 10 elements, if you have more elements, the list will have to resize its internal pool. By setting the size, you can prevent the resize and make your code faster.
//[...]
int totalPlayers = scanner.nextInt();
ArrayList<Integer> playerScores = new ArrayList<>(totalPlayers);
//[...]
int aliceTotalScores = scanner.nextInt();
ArrayList<Integer> aliceScores = new ArrayList<>(aliceTotalScores);
//[...]

Always use the base class / interface in the left part of the variable when possible
By setting the java.util.List interface in the variable part, this could make the code more refactorable in the future, since you could easily change the list type without changing everything (inheritance).
Before
 ArrayList<Integer> playerScores = new ArrayList<>(totalPlayers);

After
 List<Integer> playerScores = new ArrayList<>(totalPlayers);

Replace the for loop with an enhanced 'for' loop
In your code, you don’t actually need the index provided by the loop, you can the enhanced version.
Before
for (int i = 0; i < aliceTotalScores; i++) {
   finalScores.add(aliceScores.get(i));
   finalScores.sort(Collections.reverseOrder());
   Set<Integer> set = new LinkedHashSet<>(finalScores);
   finalScores.clear();
   finalScores.addAll(set);
   System.out.println(finalScores.indexOf(aliceScores.get(i)) + 1);
}

After
for (Integer aliceScore : aliceScores) {
   finalScores.add(aliceScore);
   finalScores.sort(Collections.reverseOrder());
   Set<Integer> set = new LinkedHashSet<>(finalScores);
   finalScores.clear();
   finalScores.addAll(set);
   System.out.println(finalScores.indexOf(aliceScore) + 1);
}

Extract some of the logic to methods.
To remove some of the code of the main method, you could move both list instantiation (playerScores & aliceScores) to methods. Since the logic is similar, you could make only one method to read the inputs to a list and reuse the same for both cases.
public static void main(String[] args) {
   List<Integer> playerScores = getPlayerScores(scanner);
   List<Integer> aliceScores = getAliceScores(scanner);
}

private static List<Integer> getAliceScores(Scanner scanner) {
   return getIntegers(scanner);
}

private static List<Integer> getPlayerScores(Scanner scanner) {
   return getIntegers(scanner);
}

private static List<Integer> getIntegers(Scanner scanner) {
   int size = scanner.nextInt();
   List<Integer> integers = new ArrayList<>(size);
   for (int i = 0; i < size; i++) {
      integers.add(scanner.nextInt());
   }
   return integers;
}

or
public static void main(String[] args) {
   List<Integer> playerScores = getIntegers(scanner);
   List<Integer> aliceScores = getIntegers(scanner);
}

private static List<Integer> getIntegers(Scanner scanner) {
   int size = scanner.nextInt();
   List<Integer> integers = new ArrayList<>(size);
   for (int i = 0; i < size; i++) {
      integers.add(scanner.nextInt());
   }
   return integers;
}

Refactored code
public static void main(String[] args) {
   Scanner scanner = new Scanner(System.in);
   List<Integer> playerScores = getPlayerScores(scanner);
   List<Integer> aliceScores = getAliceScores(scanner);
   List<Integer> finalScores = new ArrayList<>(playerScores);

   for (Integer aliceScore : aliceScores) {
      finalScores.add(aliceScore);
      finalScores.sort(Collections.reverseOrder());
      Set<Integer> set = new LinkedHashSet<>(finalScores);
      finalScores.clear();
      finalScores.addAll(set);
      System.out.println(finalScores.indexOf(aliceScore) + 1);
   }
}

private static List<Integer> getAliceScores(Scanner scanner) {
   return getIntegers(scanner);
}

private static List<Integer> getPlayerScores(Scanner scanner) {
   return getIntegers(scanner);
}

private static List<Integer> getIntegers(Scanner scanner) {
   int size = scanner.nextInt();
   List<Integer> integers = new ArrayList<>(size);
   for (int i = 0; i < size; i++) {
      integers.add(scanner.nextInt());
   }
   return integers;
}


Returning containers from function - C++ I've been trying to implement a few interfaces for processing data. And most of the time I do new of container and return a smart pointer to this container. Something like this:

class GenericUtil
{
    private:
        // Some members here

    public:
        unique_ptr<vector<string>> BreakStringOnDelimiter( string& dataStr );
};

But recently I came across "The c++ Programming Language" book and observed that Stroustrup seems to be suggesting to return containers just like that. But some other cases, he also demonstrates use of smart pointers for the same. So I have been just wondering what would be most efficient and maintainable way (plus a style which doesn't come as surprise to other fellow devs) of doing this?

unique_ptr<vector<string>> BreakStringOnDelimiter( string& dataStr );

or

vector<string> BreakStringOnDelimiter( string& dataStr );

or

vector<string>* BreakStringOnDelimiter( string& dataStr );

or something else?

Please suggest which one and why?

Thanks in advance!

In C++ 11 you probably would use

vector<string> BreakStringOnDelimiter( const string& dataStr );

The container classes can be very efficiently move constructed (this is a new and smart feature of C++ 11 which can avoid temporary objects in many cases), which means that on return no expensive copy construction of the containers (and the strings in the container) is needed. Essentially only the some pointers are passed around on return.

Alternatively the solution with unique_ptr would also work, but is in my opinion not as good (and probably not really more efficient).

Given these alternative options, returning a bare pointer is not recommended.

For maximum efficiency you should consider using the emplace function when filling the vector. This avoids additional copies of string.

Also probably const string & as argument is better suited.


Given a string, find the length of the longest substring without repeating characters I wrote an algorithm that solves this problem but I would love to get some feedback on it (especially since I'm not so confident in my Big \$O\$ skills).

Do you think this is a good quality algorithm? Is it efficient? What would be your impression if you were an interviewer? I really want to improve as a programmer so any help is appreciated!

def lengthOfLongestSubstring(self, word):
    longest =0
    currentLength = 0

    if(len(word) > 0):
        currentLength = 1
        longest = 1
        dict = {word[0] : 0}
        i = 1

        while i < len(word):
            if (dict.has_key(word[i])):
                i = dict[word[i]]+1
                dict.clear()
                if (longest < currentLength):
                    longest= currentLength
                currentLength = 0
            else:
                dict[word[i]] = i
                currentLength = currentLength + 1
                i = i + 1

        if (longest < currentLength):
            longest= currentLength

    return longest

The algorithmic complexity of your solution is \$O(N*C)\$ where \$N\$ is the number of characters in the string and \$C\$ is the size of the alphabet. Assuming lowercase ASCII characters, that's a linear algorithm and will probably be good enough for most things. That being said, it is possible to solve the same problem in \$O(N)\$.

Before that though, your current solution is hard to read. So let's simplify it.

If most of your code is inside an if statement, use a guard instead.
Don't use generic names like dict for variables. Especially not if the name is already taken. dict is a class in python.
You can use in and not in operators instead of has_key. e.g, if key in dictionary:
Use functions like max and min instead of conditionals. Actually, just avoid conditionals wherever possible. The more nested your code is, the harder it is to read.

Here's my take on the same program:

def lengthOfLongestSubstring(self, word):
    if not word: return 0

    currentLength = 1
    longest = 1
    lastSeen = {word[0] : 0}
    i = 1

    while i < len(word):
        letter = word[i]
        if letter in last_seen:
            i = last_seen[letter] + 1
            last_seen.clear()
            longest = max(longest, currentLength)
            currentLength = 0
        else:
            last_seen[letter] = i
            currentLength += 1
            i += 1

    longest = max(longest, currentLength)

    return longest

While it's a bit better, I still don't like it. Jumping back and forth is hard to wrap your head around, and it would be nice if the algorithm didn't do that.

Let's try brute force:

def lengthOfLongestSubstring(self, word):
    n = len(word)
    longest = 0
    for i in range(n):
        seen = set()
        for j in range(i, n):
            if word[j] in seen: break
            seen.add(word[j])
        longest = max(len(seen), longest)
    return longest

This one is pretty easy to understand and is still \$O(N * C)\$ in the worst case. Practically this will generally be slower that your solution, but not by that much.

Is there a way to get both readability and speed though? Yep. We don't need to go that far from your idea either. Just a couple minor changes.

Let's think about left most character that you can still include in a replication free substring while going right. Clearly, this character must be to the right of any character that has been seen again.
If we know the position of this character than the longest substring ending at the current index is easy to calculate. Here's an implementation:

def lengthOfLongestSubstring(self, word):
    # Initially we can go as far to the left as we want
    left_most_valid = 0
    longest = 0
    last_seen = {}

    for i, letter in enumerate(word):
        if letter in last_seen:
            # left_most_valid must be greater than any position which has been seen again
            left_most_valid = max(left_most_valid, last_seen[letter] + 1)
        last_seen[letter] = i

        # Length of substring from left_most_valid to i is i - left_most_valid + 1
        longest = max(longest, i - left_most_valid + 1)

    return longest

This is an \$O(n)\$ algorithm and about as fast as reading the string from input.


Weather API implementation This class uses the openweathermap API to receive information about the current weather and forecasts. I am looking for any type of advice, but primary for structure and style improvement suggestions:
This method builds the the API URL:

private Uri BuildQueryURL(Interval requestType, NameValueCollection parameters)
{
    if (parameters == null) parameters = new NameValueCollection();
    string relativePath = "";
    //set query//
    switch (requestType)
    {
        case Interval.Current:
            relativePath = "weather";
            break;
        case Interval.Forecast:
            relativePath = "forecast";
            break;
    }
    //add api key//
    parameters.Add("APPID", ApiKey);
    //if needed, add unit parameter//
    if (Units != Units.Default) parameters.Add("units", Units.ToString());
    //build uri
    var builder = new UriBuilder();
    builder.Scheme = "http://";
    builder.Host = "api.openweathermap.org";
    builder.Path = Path.Combine("data/2.5", relativePath);
    builder.Query = string.Join("&", parameters.AllKeys.Select((name) => string.Format("{0}={1}", HttpUtility.UrlEncode(name), HttpUtility.UrlEncode(parameters[name]))));
    return builder.Uri;
}

The actual query is done by following method:

private RequestStatus Execute(Uri queryURI, out string queryResponse)
    {
        queryResponse = null;
        var status = RequestStatus.OK;
        try
        {
            queryResponse = _client.DownloadString (queryURI);
            //this methods parses the code parameter of the query response//
            status = CheckResponseCode(queryResponse);
        }
        catch(WebException) {
            status = RequestStatus.Failed;
        }
        return status;
    }

Here is the deserialization of the received data:

 private CurrentWeather DeserializeCurrentWeather(string queryResponse)
    {
        var currentWeather = JsonConvert.DeserializeObject<CurrentWeather>(queryResponse);
        currentWeather.Units = this.Units;
        currentWeather.Country = CountryCodeHelper.ConvertToName((string)JObject.Parse(queryResponse).SelectToken("sys.country"));
        return currentWeather;
    }

This is called by the consumer:

public RequestStatus GetWeatherByZip(int zip, string countryCode, out CurrentWeather result)
    {
        return GetWeather(new NameValueCollection() { { "zip", CombineQueryCountryCode(zip.ToString(), countryCode) } }, out result);
    }
public RequestStatus GetWeatherByCity(string city, string countryCode, out CurrentWeather result)
    {
        return GetWeather(new NameValueCollection() { { "q", CombineQueryCountryCode(city,countryCode) } }, out result);
    }
//...//
private RequestStatus GetWeather(NameValueCollection parameters, out CurrentWeather result)
    {
        string queryResponse;
        RequestStatus status = RequestStatus.OK;
        status = Execute(BuildQueryURL(Interval.Current, parameters), out queryResponse);
        result = DeserializeCurrentWeather(queryResponse);
        return status;
    }

I excluded the async methods and forecast methods. The complete source can be found at the GitHub page.

BuildQueryURL method

This method is in a very bad style. It's supposed to build a query url but you give it a name-value-collection that you modify it inside the method!

You should split it into two methods.

The first one should only UpdateParameters and the other one BuildQueryURL only build the url without modifying any other values and causing unexpected behaviour. I'd be really supprised if I used it and found that it actually changed the parameters too although it's name doesn't suggest anything like that.

Interval enum

If this is your enum then make the values exactly like the relative path so that you can just call .ToString() on it without a switch. If not then I'd use a dictionary to get rid of the switch.

Execute & GetWeather methods

Usually when we do somehting like this where we have an out argument and return a bool or another type of a result success indicator we name the method TryDoSomething so in your case the method should be called TryExecuteQuery and TryGetWeather


Locating the Bash history file for a user This looks quite verbose to me, using the command substitution and echo:

# Source file
source=$( [[ ! -z "$1" ]] && echo "/home/$1" || echo '/root' )
source="$source"/.bash_history
[[ ! -f "$source" ]] && echo "History file '$source' not found." 1>&2 && exit 1;

# Am I dealing with root
isRoot=$([[ "$source" =~ ^/root ]] && echo 1 || echo 0 )

Is there a cleaner (maybe faster?) manner to assign a value to variable depending on some condition?

A clearer way would be to establish a default for the user, then locate the Bash history file for that user.

You're making the assumption that the user's home directory will be in /home, and that a root user's home directory will be /root.  That's a convention for Linux, but it won't be true at larger Linux installations, and it isn't the convention for other operating systems.  Here's one way to do the lookup for systems that use GNU libc:

user="${1-root}"
histfile="$(getent passwd "$user" | cut -d: -f6)/.bash_history"

A cross-platform way is to use tilde expansion, though the eval could be dangerous if the input is untrustworthy:

histfile=$(eval echo "~${1-root}")/.bash_history

The isRoot check should be done on the username (or UID) instead of the home directory.  It feels very unconventional to assign a boolean variable in Bash.  Keep in mind that the convention is that zero statuses are considered "true" and non-zero statuses are considered "false".  I'd advise you not to write that statement at all, but can't really advise you what to do instead since you haven't shown us what you intend to do with that variable.


Splitting a string into groups of up to 3 numbers, or one letter and up to 3 numbers There was an interesting idea brought up in The 2nd Monitor where one of our regulars was trying to split a bunch of strings into a specific format.

The format should be similar to the following:

A000
A00
900
90

Where A is any alphabetical letter, 0 is any number, and 9 is any number 1-9. No result string should begin with 0, it should always begin with A-Z or 1-9, and all alphabet characters will always be capitalized.

The input comes in a format similar to any of the following:

Input         | Result
900A000       | 900, A000
900900        | 900, 900
90            | 90
99099         | 990, 99
A009A09       | A009, A09
A009A09900    | A009, A09, 900
A0990A09900   | A09, 90, A09, 900
A099090A09900 | A09, 90, 90, A09, 900
A09A990       | A09, A990
A990          | A990

The input will always parse to a valid group of strings.

The method:

public List<string> SpecialSplit(string input)
{
    var result = new List<string>();

    var currentString = new StringBuilder(4);
    for (var i = 0; i < input.Length; i++)
    {
        var c = input[i];

        if (currentString.Length > 0)
        {
            // Determine whether we're at constraints or not.
            var firstCharLetter = currentString[0] >= 'A' && currentString[0] <= 'Z';
            var atMaxLetterLength = firstCharLetter && currentString.Length == 4;
            var atMaxNumberLength = !firstCharLetter && currentString.Length == 3;

            // Split if at max letter/number length, or if we're on a letter.
            var mustSplit = atMaxLetterLength || atMaxNumberLength || c >= 'A' && c <= 'Z';

            if (mustSplit)
            {
                // If we must split our string, then verify we're not leaving an orphaned '0'.
                if (c == '0')
                {
                    // Go back a letter, take it out of the new string, and set our `c` to it.
                    i--;
                    currentString.Length--;
                    c = input[i];
                }

                // Add and clear the string to our result.
                result.Add(currentString.ToString());
                currentString.Clear();
            }
        }

        // Add our `c` to the string.
        currentString.Append(c);
    }

    // Add our string to the result.
    result.Add(currentString.ToString());

    return result;
}

And the test code:

var tests = new string[] {
    "909A999",
    "909999",
    "90",
    "99099",
    "A009A09",
    "A009A09900",
    "A0990A09900",
    "A099090A09900",
    "A09A990",
    "A990"
};

var resultStrings = new List<string>();

foreach (var test in tests)
{
    var split = SpecialSplit(test);
    Console.WriteLine(test + ": {" + string.Join(", ", split) + "}");
    resultStrings.AddRange(split);
}

if (resultStrings.Any(x => x[0] == '0'))
    Console.WriteLine("Test failed");
else
    Console.WriteLine("Test passed");

right ... "tempted to solve this with a regex" doesn't quite cut it. There is a regex that kinda sorta somewhat works, but I didn't get it to do exactly what I want.
Doesn't matter though, because I can make it do what I wanted, namely spit out the results directly as separate groups. Instead I got it to match the "last result" an:

private readonly Regex someNiceName = new Regex(@"^((?:[A-Z]\d|[1-9])\d{1,2})+$", RegexOptions.Compiled);

public List<string> SpecialSplit(string input)
{
    if (input == "")
    {
       return new List<string>(); // base case for recursion
    }
    var match = someNiceName.Match(input);
    string last = match.Groups[1].Value;
    var precedessors = SpecialSplit(input.Substring(0, match.Groups[1].Index));
    precedessors.Add(last);
    return precedessors;
}

With a bit of luck we don't actually need this mess, but can rely on Captures instead:

public List<string> SpecialSplit(string input)
{
    var match = someNiceName.Match(input);
    var fullMatch = match.Groups[0];
    return fullMatch.Captures().Select(c => c.Value).ToList();
}

unfortunately I do not have C# available to check right now, but that might work better :)


Python PBKDF2 using core modules I'm trying to implement pbkdf2 in Python 2.7/3.4 without needing the pbkdf2 module. (I'm also trying to avoid using classes.)

Any insight would be appreciated:

from binascii import hexlify, unhexlify
import hmac, struct, hashlib, sys

is_python2 = True if sys.version_info.major == 2 else False

def pbkdf_two(passwd, salt, iters=2048, keylen=64, digestmod=hashlib.sha512):
    """
    >>> hexlify(pbkdf_two(b'All n-entities must communicate with other n-entities via n-1 entiteeheehees', unhexlify('1234567878563412'), 500, 16, hashlib.sha1))
    '6a8970bf68c92caea84a8df285108586'
    """
    dgsz = digestmod().digest_size if callable(digestmod) else digestmod.digest_size
    if keylen is None: keylen = dgsz
    # Helper function which copies each iteration for h, where h is an hmac seeded with password
    def pbhelper(h, salt, itercount, blocksize):
        def prf(h, data):
            hm = h.copy()
            hm.update(data)
            return hm.digest()
        U = prf(h, salt + struct.pack('>i', blocksize))
        T = U
        for j in range(2, itercount+1):
            U = prf(h, U)
            T = "".join([chr( ord(x) ^ ord(y) ) for (x, y) in zip( T, U )]) \
                  if is_python2 else bytes([x ^ y for (x, y) in zip(T, U)])    # XORing
        return T
    L = int(keylen/dgsz) # L - number of output blocks to produce
    if keylen % dgsz != 0: L += 1
    h = hmac.new(key=passwd, msg=None, digestmod=digestmod )
    T = b""
    for i in range(1, L+1):
        T += pbhelper(h, salt, iters, i)
    return T[:keylen]

1. Review

The docstring doesn't explain what the function does, or how to call it, or what it returns. There's just a doctest.
If the code were reformatted to fit in 79 columns, as recommended by the Python style guide (PEP8), then we wouldn't have to scroll it horizontally to read it here at Code Review.
hexlify and unhexlify are only used by the doctest, so they could be included there.
sys is not used, so the import could be omitted.
The key derivation function is called PBKDF2, so I think the function would be better named pbkdf2, not pbkdf_two.
I don't think that having default values for arguments to this function is a good idea. The caller needs to think about these values, not rely on the defaults. Note that RFC 2898 doesn't specify any particular values.
We're not running out of letters, so why not password instead of passwd and digest_size instead of dgsz?
The helper function pbhelper is called from only one place in the code, so there's nothing to be gained by making it into a local function.
The fourth argument to pbhelper is named blocksize, but this is very misleading: it's actually the (1-based) index of the block.
The iteration variable j is not used in the body of the loop. It's conventional to name such a variable _.
Since j is not used, it doesn't matter what values it takes, and so it's simpler to use range(iters-1) instead of range(2, iters+1).
Since h is always the same, there's no need for it to be a parameter to the prf function.
The msg argument to hmac.new defaults to None so there's no need to specify it.
If the code used bytearray instead of bytes then it would be portable between Python 2 and 3 without needing a version test.
Instead of:

L = int(keylen/dgsz) # L - number of output blocks to produce
if keylen % dgsz != 0: L += 1

the number of blocks can be computed like this, using the floor division operator //:

L = (keylen + dgsz - 1) // dgsz

But even more simply, why not just iterate until the result is long enough? That way you wouldn't have to compute digest_size, and it would have the advantage that in Python 3.4 or later, the caller could pass in the name of the digest algorithm, just as for hmac.new.

2. Revised code

import hmac
import struct

def pbkdf2(password, salt, iters, keylen, digestmod):
    """Run the PBKDF2 (Password-Based Key Derivation Function 2) algorithm
    and return the derived key. The arguments are:

    password (bytes or bytearray) -- the input password
    salt (bytes or bytearray) -- a cryptographic salt
    iters (int) -- number of iterations
    keylen (int) -- length of key to derive
    digestmod -- a cryptographic hash function: either a module
        supporting PEP 247, a hashlib constructor, or (in Python 3.4
        or later) the name of a hash function.

    For example:

    >>> import hashlib
    >>> from binascii import hexlify, unhexlify
    >>> password = b'Squeamish Ossifrage'
    >>> salt = unhexlify(b'1234567878563412')
    >>> hexlify(pbkdf2(password, salt, 500, 16, hashlib.sha1))
    b'9e8f1072bdf5ef042bd988c7da83e43b'

    """
    h = hmac.new(password, digestmod=digestmod)
    def prf(data):
        hm = h.copy()
        hm.update(data)
        return bytearray(hm.digest())

    key = bytearray()
    i = 1
    while len(key) < keylen:
        T = U = prf(salt + struct.pack('>i', i))
        for _ in range(iters - 1):
            U = prf(U)
            T = bytearray(x ^ y for x, y in zip(T, U))
        key += T
        i += 1

    return key[:keylen]


Cleaning up file resources in a DRY manner I've written a function that generates a report, and then sends it as an attachment to an email.  I don't want to clutter the function with a lot of calls to close up the filesystem, if there was an error along the way.  What do you all think?

// attempt to generate the report, any error will be wrapped in Failure(ex)
val op1 = Try {
  DB.withConnection{ implicit c =>
    generateReport(command.programId, token.claims)(output, c)
  }
}

// use (abuse?) pattern matching to send the report if it generated succesfully
val op2 = op1 match {
  case Success(_) => Try(sendReport(Address(user.email, user.first), file, command.created))

  // if the op1 value is of type Failure(ex), carry it forward until after the temp files have been removed
  case Failure(ex) => Failure(ex)
}

// the operations have either succeeded or failed, so clean up the filesystem
cleanUp(output, file)

// throw any exception that occurred along the way
op2.get

Assume that throwing exceptions is a legitimate design choice for this function, the calling process expects exceptions to be thrown and handles them gracefully.

I think this can be written more cleanly with flatMap

  flatMap[U](f: (T) ⇒ Try[U]): Try[U]

  Returns the given function applied to the value from this Success or returns this if this is a Failure.

val isReportSent = Try {
 ...
}.flatMap(_ => Try { sendReport(...) })

cleanUp(output, file)

isReportSent.get


Merging worksheets and using find/replace Of all the macros that I put into heavy rotation these days, this one is running the slowest. ~4-5 seconds depending on the size of the files. It's not a lot but I'd like to know why code 16x as long is running much more instantly.

The code tries to merge documents (usually 2 excel docs out of at most 5) depending on their names and then rename those to exactly what I need. Then, another big issue, is using find/replace to fix a bunch of Unicode/character issues. I cant help but think that could be handled better.

I'd like to find out where the bottlenecks in this code are, how to handle these Unicode issues, perform the Find/replace better, and all in all how to execute better VBA practices.

Option Explicit
Sub MergeBooks()
Dim wb As Workbook
Dim ws As Worksheet

On Error GoTo Handler:
Application.ScreenUpdating = False
For Each wb In Application.Workbooks
    If wb.Name <> "CompanyBook.xlsm" Then
        If FindString(wb.Name, "Report2") Then
            wb.Worksheets.Move after:=Workbooks("CompanyBook.xlsm").Sheets("Aggregate")
        ElseIf FindString(wb.Name, "Report1") Then
            wb.Worksheets.Move after:=Workbooks("CompanyBook.xlsm").Sheets("Aggregate")
        End If
    End If
Next

For Each ws In Workbooks("CompanyBook.xlsm").Worksheets
    If FindString(ws.Name, "Report2") Then
        ws.Name = "Report2"
    ElseIf FindString(ws.Name, "Report1") Then
        ws.Name = "Report1"
    End If
Next ws

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.Replace What:="&amp;", Replacement:="&", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report1").Cells.Replace What:="&quot;", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€™", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€¦", Replacement:="…", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="Â£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="â€˜", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

Application.ScreenUpdating = True
Exit Sub
Handler:
Application.ScreenUpdating = True
MsgBox "Please make sure that one and only one type of each database file is open.", vbExclamation, "Merge Documents"
End Sub
Function FindString(strCheck As String, strFind As String) As Boolean
Dim intPos As Integer
    intPos = InStr(strCheck, strFind)
    FindString = intPos > 0
End Function

To supplement Gaffi's suggestions, I think you would benefit from changing this:

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.Replace What:="&quot;", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€™", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€¦", Replacement:="…", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="Â£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="â€˜", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

To this:

Dim r1 As Excel.Range, r2 As Excel.Range
Set r1 = Workbooks("CompanyBook.xlsm").Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants)
Set r2 = Workbooks("CompanyBook.xlsm").Worksheets("Report2").Cells.SpecialCells(xlCellTypeConstants)

With r1
    .Replace What:="&amp;", Replacement:="&", LookAt:=xlPart, MatchCase:=False
    .Replace What:="&quot;", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
End With

With r2
    .Replace What:="â€¦", Replacement:="…", LookAt:=xlPart, MatchCase:=False
    .Replace What:="Â£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
End With

This way, you narrow down the selection to only cells that have content for Excel to find/replace. Also, because you set the range to a variable once, Excel doesn't have to search through all cells multiple times like it is now.


Methods to create C# dictionaries, with static keys/values, in fluent notation I created these little utility methods to build fluent C# dictionaries. The value declarations shall be quick to write and easy to comprehend when somebody views the code. Usage conditions are as follows:

many keys point to the same values
group keys pointing to one value. Declare value only once for better visual comprehension.
only relatively few values (usually 10 to 20, rarely up to 100)
main application purpose: replace switch-case blocks in legacy code

The keys/values can also be added otherwise, such as from all constants or Resource content, and can be of any type useful to be declared in code.

In addition, I created an extension convenience method: simply .ToDictionary(), instead of constructor lambdas kv => kv.Key, kv => kv.Value

Is this good, or are there any problems or better ideas? What about performance when used frequently, compared to switch-case?

Usage/test:

class Program
{
    public const string RoadVehicles = "RoadVehicles";
    public const string RailVehicles = "RailVehicles";
    public const string Watercraft = "Watercraft";
    public const string Aircraft = "Aircraft";

    public static readonly IReadOnlyDictionary<string, string> GroupsForVehicles =
        FluentDictionaries.KeysToValue(
            RoadVehicles, "Car", "Truck", "Tractor", "Motorcycle")
        .KeysToValue(
            RailVehicles, "Locomotive", "Railcar", "Powercar", "Handcar")
        .KeysToValue(
            Watercraft, "Ship", "Sailboat", "Rowboat", "Submarine")
        .KeysToValue(
            Aircraft, "Jetplane", "Propellerplane", "Helicopter", "Glider", "Balloon")
        .ToDictionary();

    static void Main(string[] args)
    {
        foreach (var key in GroupsForVehicles.Keys.OrderBy(key => key))
        {
            Console.WriteLine(key + ": " + GroupsForVehicles[key]);
        }
        Console.ReadLine();
    }
}

The fluent methods:

public static class FluentDictionaries
{
    public static IEnumerable<KeyValuePair<TKey, TValue>> KeysToValue<TKey, TValue>(TValue value, params TKey[] keys)
    {
        return keys.Select(key =>
            new KeyValuePair<TKey, TValue>(key, value));
    }

    public static IEnumerable<KeyValuePair<TKey, TValue>> KeysToValue<TKey, TValue>(
        this IEnumerable<KeyValuePair<TKey, TValue>> previous, TValue value, params TKey[] keys)
    {
        return previous.Concat(keys.Select(key =>
            new KeyValuePair<TKey, TValue>(key, value)));
    }

    public static Dictionary<TKey, TValue> ToDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> keyValuePairs)
    {
        return keyValuePairs.ToDictionary(kv => kv.Key, kv => kv.Value);
    }
}

Test output of the program above:

Balloon: Aircraft

Car: RoadVehicles

Glider: Aircraft

Handcar: RailVehicles

Helicopter: Aircraft

Jetplane: Aircraft

Locomotive: RailVehicles

Motorcycle: RoadVehicles

Powercar: RailVehicles

Propellerplane: Aircraft

Railcar: RailVehicles

Rowboat: Watercraft

Sailboat: Watercraft

Ship: Watercraft

Submarine: Watercraft

Tractor: RoadVehicles

Truck: RoadVehicles

This sounds like a good idea but I don't think it is, at least not with the current method names that I agree with @denis are very confusing.

The name KeysToValue should actually be AddValueWithKeys because this is the order of parameters and it better suggests that the first parameter is a value and not a key. KeysToValue sounds like a query that gets something or converts keys to values etc.

I also find the extension should extend the dictionary and not some arbitrary IEnumerable<KeyValuePair<TKey, TValue>> because if it will throw a duplicate key exception, it will be hard to find where it happened and it's of more use if extending a dictionary. If it however extends the IEnumerable then the name should be Concat.

The improved version could look like this:

public static Dictionary<TKey, TValue> AddValueWithKeys<TKey, TValue>(
    this Dictionary<TKey, TValue> dictionary,
    TValue value,
    params TKey[] keys)
{
    foreach (var key in keys) dictionary.Add(key, value);
    return dictionary;
}

and is much easier to use as it now requries only a single extension

public static readonly IReadOnlyDictionary<string, string> GroupsForVehicles =
    new Dictionary<string, string>()
        .AddValueWithKeys(RoadVehicles, "Car", "Truck", "Tractor", "Motorcycle");


Multilingual app without localizing I'm making an iOS app which supports multiple languages but not in the built-in localized way. In iOS, for localization to work, the user has to change the device's language. But in the app I'm working on, the user needs to be able to choose the language to effect only this app. So for example, I can have the device's language set to English but the user can choose Spanish to be set as the language within the app.

I took some concepts from iOS localization and added some things on my own. First I created plists to contain strings for each language I support.

English

Spanish

Next I created a singleton which handles the setting on the chosen language and serving the strings based on that selected language. I use a third-party library called SwiftyPlist for easier manipulation of plists.

enum Language: String {
    case english = "en"
    case spanish = "es"
}

class LanguageManager {
    static let shared = LanguageManager()

    private var plist: Plist!

    // Get/Set the language of choice
    var language: Language? {
        set {
            UserDefaults.standard.set(newValue?.rawValue, forKey: "Language")
            reloadPlist()
        }
        get {
            if let code = UserDefaults.standard.object(forKey: "Language") as? String {
                return Language(rawValue: code)
            } else {
                return nil
            }
        }
    }

    private init() {
        reloadPlist()
    }

    // Reload the corresponding plist
    private func reloadPlist() {
        if language == nil || language! == .english {
            let dict = NSDictionary(contentsOfFile: Bundle.main.path(forResource: "English", ofType: "plist")!)
            plist = Plist(plistObject: dict)
        } else if language! == .spanish {
            let dict = NSDictionary(contentsOfFile: Bundle.main.path(forResource: "Spanish", ofType: "plist")!)
            plist = Plist(plistObject: dict)
        }
    }

    var name: String {
        return plist["YOUR_NAME"]!.string!
    }

}

Now I can set the chosen language like this.

LanguageManager.shared.language = .spanish

And I can simply access the strings like so.

LanguageManager.shared.name

I feel like this is a good, scalable approach to my requirement. But I'd like to see if it has room to improvement.

Demo project

I did something like this. You can also just use a Strings file. What I did was create a spreadsheet in the Google Drive with all the translations (one column for the identifier BUTTON_TITLE_OK and one each language the app supports). It's then very easy to make a little formula in Google Drive that takes all the identifiers, and turns it into an enum. Like so:

    enum LocalizedStringIdentifier: String {
        case BUTTON_TITLE_OK
    }

    func string(_ identifier: LocalizedStringIdentifier) -> String? {
        guard let value = plists[identifier.rawValue] else { return nil }
        return value
    }

The advantage of this that you get code completion in Xcode and thus less chance of mistakes. Usage:

LanguageManager.shared.string(.BUTTON_TITLE_OK)

The advantage of the spreadsheet approach is that you have all localizations combined so you have a better overview (and it is also easier to share with your client so that they can provide you with the right translations, unless this is your own project).

Side note: If you do use plists, you should try use the new Codable protocol to load Plists directly into your models.


Is there a way to avoid repetitive code in the following switch statement? I have the following method, which is called every time the user clicks a checkbox within a Datagrid. Each column within this Datagrid has a box in its header, which checks/unchecks all child checkboxes in the respective column. This header checkbox will be updated according to the child checkboxes by the method.

My problem is that there is a lot of repetitive code in it since we do the same operation for each property (column) based on the checkbox that was clicked.

Here is the code:

    /// <summary>
    /// Sets the Columnsheadercheckbox based on the individual checkbox values of the respective filter
    /// </summary>
    private void SetHeaderCheckbox(string filterProperty = null)
    {
        if (GridData.Count == 0) return;
        var filter = GridData[0];
        DataGridDropdownProperty found;
        if (filterProperty == null) filterProperty = "all";
        switch (filterProperty)
        {
            case "SuperImpose":
                found = GridData.FirstOrDefault(f => f.SplitOver != filter.SplitOver);
                if (found == null)
                {
                    SuperimposeHeaderCheckbox = filter.SplitOver;
                }
                else
                {
                    SuperimposeHeaderCheckbox = null;
                }
                break;
            case "Normalize":
                found = GridData.FirstOrDefault(f => f.ToNormalize != filter.ToNormalize);
                if (found == null)
                {
                    NormalizeByHeaderCheckbox = filter.ToNormalize;
                }
                else
                {
                    NormalizeByHeaderCheckbox = null;
                }
                break;
            case "Legend":
                found = GridData.FirstOrDefault(f => f.ToLegend != filter.ToLegend);
                if (found == null)
                {
                    LegendHeaderCheckbox = filter.ToLegend;
                }
                else
                {
                    LegendHeaderCheckbox = null;
                }
                break;
            case "all":
                // Superimpose
                found = GridData.FirstOrDefault(f => f.SplitOver != filter.SplitOver);
                if (found == null)
                {
                    SuperimposeHeaderCheckbox = filter.SplitOver;
                }
                else
                {
                    SuperimposeHeaderCheckbox = null;
                }
                // Normalize
                found = GridData.FirstOrDefault(f => f.ToNormalize != filter.ToNormalize);
                if (found == null)
                {
                    NormalizeByHeaderCheckbox = filter.ToNormalize;
                }
                else
                {
                    NormalizeByHeaderCheckbox = null;
                }
                // Legend
                found = GridData.FirstOrDefault(f => f.ToLegend != filter.ToLegend);
                if (found == null)
                {
                    LegendHeaderCheckbox = filter.ToLegend;
                }
                else
                {
                    LegendHeaderCheckbox = null;
                }
                break;
        }
    }

My question is, if this kind of code is 'OK' or if it is, this is terrible practice, and there should be a more elegant way around it. (btw. Ignore the hardcoded strings. This is to illustrate the example)

Replace the if-else statements under each case with inline if statements. You can remove the if-else repetitions. For example..

SuperimposeHeaderCheckbox = found ? null : filter.SplitOver;

You can use turn filterProperty into an enumeration (instead of a string).

enum flags {SuperImpose=1, Normalize=2, Legend=4, all=7};

Finally, replace the switch-case statement with simple if statements. You can remove the repetitions for the case all.

if (filterProperty & flags.SuperImpose) {
 // your code here
}
if (filterProperty & flags.Normalize) {
 // your code here
}
if (filterProperty & flags.Legend) {
 // your code here
}
// No need for a fourth if statement.

If you furthur want to reduce the number of the above if statements, you can map the properties in filter to the values in the enumeration and loop through the map.


Thread overkill with async/await Consider the following code.

public async void Connect()
{
  bool success = await ConnectAsync();
  if (success) NotifyUserOfSuccess();
}

public async Task<bool> ConnectAsync()
{
  var t = await Task.Run(()=>
    try {
       ConnectSynchronously();
    } catch (Exception e)
    {
       return false;
    }
    return true;
   );
}

When analyzing this code, I come to the conclusion that I potentially am kicking off three threads.
Assume that both await's will not already be completed when called, then the first thread is kicked off by the await ConnectAsync(); which in turn kicks off the await Task, and the Task.Run will kick off the function inside in a third thread.
I really only need one thread, namely the one in Connect().

How can I improve the design?

As Connect() contains the await keyword, shouldn't it really be called ConnectAsync() regardless of its return value?

You have a wrong assumption here. await never starts a new thread! Strictly speaking even Task.Run might not start a new thread since it uses the thread pool.

See this answer for more details on await: https://stackoverflow.com/questions/16978904/async-programming-control-flow/16980183#16980183


Convert sorted linkedlist into balanced binary search tree Convert a sorted linkedlist into a balanced binary search tree. Looking for code-review, optimizations, and best practices.

public class SortedLinkedListToBalancedBST<T> {

    private TreeNode<T> root;

    private static class TreeNode<T> {
        TreeNode<T> left;
        T item;
        TreeNode<T> right;
    };

    public void convert(LinkedList<T> ll) {
        Iterator<T> itr = ll.iterator();
        root = inorder(0, ll.size() - 1, itr);
    }

    private TreeNode<T> inorder(int lb, int hb, Iterator<T> itr) {
        if (lb > hb) return null;
        // same as (lb+hb)/2, avoids overflow
        int mid = lb + (hb - lb) / 2;

        final TreeNode<T> treeNode = new TreeNode<T>();
        treeNode.left = inorder(lb, mid - 1, itr);
        treeNode.item = itr.next();
        treeNode.right = inorder(mid + 1, hb, itr);

        return treeNode;
    }

    public List<T> getInorderList() {
        final List<T> inorderList = new ArrayList<T>();
        inorder(root, inorderList);
        return inorderList;
    }

    private void inorder(TreeNode<T> node, List<T> list) {
        if (node != null) {
            inorder(node.left,  list);
            list.add(node.item);
            inorder(node.right, list);
        }
    }
}

public class SortedLinkedListToBalancedBSTTest {

    @Test
    public void testOne() {
        SortedLinkedListToBalancedBST<Integer> s = new SortedLinkedListToBalancedBST<Integer>();
        LinkedList<Integer> list1 = new LinkedList<Integer>(Arrays.asList(1, 2, 3, 4));
        s.convert(list1);
        assertEquals(list1,  s.getInorderList());
    }

    @Test
    public void testTwo() {
        SortedLinkedListToBalancedBST<Integer> s = new SortedLinkedListToBalancedBST<Integer>();
        LinkedList<Integer> list2 = new LinkedList<Integer>(Arrays.asList(1, 2, 3, 4, 5));
        s.convert(list2);
        assertEquals(list2,  s.getInorderList());
    }

    @Test
    public void testThree() {
        SortedLinkedListToBalancedBST<Integer> s = new SortedLinkedListToBalancedBST<Integer>();
        LinkedList<Integer> list3 = new LinkedList<Integer>(Arrays.asList(1, 2, 3, 4, 5, 6));
        s.convert(list3);
        assertEquals(list3,  s.getInorderList());
    }

    @Test
    public void testFour() {
        SortedLinkedListToBalancedBST<Integer> s = new SortedLinkedListToBalancedBST<Integer>();
        LinkedList<Integer> list4 = new LinkedList<Integer>(Arrays.asList(1, 2, 3, 4, 5, 6, 7));
        s.convert(list4);
        assertEquals(list4,  s.getInorderList());

    }
}

I'm going to post a simpler review than I would if I fully understood this class, but hopefully if you clarify some things, I (or others) can come back and review it more in-depth.

My main problem with this code, is that I don't know what it is trying to do. The class is called SortedLinkedListToBalancedBST, which makes me think that it is some kind of converter utility class. However, the class itself has a private member of TreeNode<T> and doesn't have any public methods which are static or return some kind of BST, so that makes me think that it is just a BST implementation which happens to know how to create a BST from a Linked List. But the class doesn't even have all the functionality of a BST!

You need to decide what this class is going to be and then name it appropriately. Is it a class which knows how to convert from a Linked List to a BST and vice-versa, or is it a BST which knows how to create a BST from a Linked List? (The current name isn't even really all encompassing since the class can get the Linked List from the BST as well.)

Without a better understanding of the purpose of this class, I can't comment on the functionality and implementation, but I can talk about the style:

Give your variables better names. ll, lb, and hb are not good names. linkedList is a better name for ll, and for the others, I honestly have no clue what they even are supposed to represent.
Give different methods different names. Your two inorder() methods do two completely different things. One adds items to a List. The other gets a tree from an ordered list.
convert() seems like it might be more appropriate as a constructor.


Finding solutions on GitHub and Stack Overflow The first part of the code opens Chrome and navigates to GitHub. If I don't find my solution, I open a new tab with ctrlt. Then my code goes to Stack Overflow and searches there for solutions.
from selenium import webdriver
from selenium.webdriver.common.keys import *
import time
import keyboard

q = input("what is your programming question : ")+(' python')
class github :
    path = "c:/users/admin/appdata/local/programs/python/python38-32/chromedriver.exe"
    driver = webdriver.Chrome(path)

    driver.get("https://github.com/")
    question = driver.find_element_by_name("q")
    time.sleep(3)

    question.send_keys(q)
    question.send_keys(Keys.RETURN)

    while True:
        if keyboard.is_pressed("ctrl+t"):
            driver.switch_to_window(driver.window_handles[-1])
            driver.get("https://stackoverflow.com/")
            question_2 = driver.find_element_by_name("q")

            question_2.send_keys(q)
            question_2.send_keys(Keys.RETURN)
            break

Selenium
Selenium imposes a lot of overhead and complexity that you don't need to deal with. If you needed to scrape, use requests + beautifulsoup; but you don't: StackExchange has a good API, so you should strongly prefer using that. It will get you results more efficiently and the results will be more accurate and less fragile.
PEP8
class github :

should be
class Github: