Update existing key value if present and then deserialize json to POCO efficiently I have a json string ProcessValue in which I need to extract a particular key (client_id) value and then I need to encode that value using HttpUtility.HtmlEncode and put it back in the same JSON string. ProcessValue is one of the header name which has a json value in it.
Below is my code which does that but I am deserializing twice looks like. Is there any way to improve below code?
ProcessValue = GetHeader(headers, PROCESS_DATA); // LINE A
if (!string.IsNullOrWhiteSpace(ProcessValue))
{
    dynamic jsonObject = Newtonsoft.Json.JsonConvert.DeserializeObject(ProcessValue);
    if (jsonObject.client_id != null)
    {
        var value = jsonObject.client_id;
        jsonObject.client_id = HttpUtility.HtmlEncode(value);
        var modifiedJsonString = Newtonsoft.Json.JsonConvert.SerializeObject(jsonObject);
        ProcessValue = modifiedJsonString;
    }
    ProcCustomer = DeserializeJson<ProcCustomer>(ProcessValue);
}

private T DeserializeJson<T>(string str) where T : new()
{
    try
    {
        return JsonSerializer.Deserialize<T>(str);
    }
    catch (Exception e)
    {
        return new T();
    }
}

Here ProcessValue at LINE A is:
{"key1":"value1","key2":"value2","key3":"value3","key4":"value4","key5":"value6","client_id":"HÃ¤nzendes"}

I am extracting client_id value and then encoding it and after that putting it back to same json. And then I am deserializing this modified json value to ProcCustomer POCO. ProcCustomer is the POCO class which holds all the fields value. So I am deserializing and serializing to get my updated json string ProcessValue and then I deserialize this json to ProcCustomer POCO class.
There are places where we use ProcessValue json string directly as it is one of the header value and using ProcCustomer POCO fields as well so that is why I was doing above things. I wanted to see if there is anything I can do to optimize my above code where I don't have to deserialize and then serialize and then again deserialize to put it in POCO?
Also I am mixing Utf8Json library with Newton json library. I prefer to use Utf8 json library for both serialization and deserialization in my above code.

Even if ProcessValue is used as string in other objects, why you don't just deserialize it once across the class, then use the POCO object directly. Then, serialize it whenever needed.
So, in your question this would be valid :
ProcessValue = GetHeader(headers, PROCESS_DATA); // LINE A

if (!string.IsNullOrWhiteSpace(ProcessValue))
{
    ProcCustomer = DeserializeJson<ProcCustomer>(ProcessValue);

    if (ProcCustomer != null && !string.IsNullOrWhiteSpace(ProcCustomer.ClientId))
    {
        ProcCustomer.ClientId = HttpUtility.HtmlEncode(ProcCustomer.ClientId);
    }
}

This way you only deserialized it once in this scope. Also, if the client_id is always needs to be encoded using HttpUtility.HtmlEncode you could modify the POCO and do this for instance :
public class ProcCustomer
{
    //.. other properties

    [JsonProperty("client_id")]
    public string ClientId
    {
        get => ClientId;
        set
        {
            if(!string.IsNullOrEmpty(value))
            {
                ClientId = HttpUtility.HtmlEncode(value);
            }
            else
            {
                ClientId = value;
            }
        }
    }
}

With this, your code will be shorter as you only need to deserialize, the rest will be updated according to the POCO logic!
Answering the comments :
working with ProcessValue as string, will make things difficult to handle. What I suggested is to use the deserialized object (which is the POCO in your case), then work with it like any object. When you want to pass it to the HttpClient header, just serialize it, to pass it as sting.
The main idea is to work with objects, and not with the strings! use the strings only when required. This way you'll have a better coding experience.
For example, we can modify the POCO and override the ToString method like this :
public class ProcCustomer
{
    public override string ToString() {
        return Newtonsoft.Json.JsonConvert.SerializeObject(this);
    }
}

Now, when we want to add it to the header we can do this (example) :
// assuming we've already updated the POCO values
// and we need to add it to the HttpClient header
using (var client = new HttpClient{ BaseAddress = baseAddress })
{
    client.DefaultRequestHeaders.Add("", ProcCustomer.ToString());

    using (var response = await client.GetAsync(Url))
    {
        var responseData = await response.Content.ReadAsStringAsync();
    }
}

If you can't modify the POCO, then you can add extensions like this :
// implement the extensions
public class JsonExtensions
{
    public T JsonDeserialize<T>(this string str) where T : class
    {
        try
        {
            return Newtonsoft.Json.JsonConvert.DeserializeObject<T>(str);
        }
        catch (Exception e)
        {
            return default;
        }
    }

    public string JsonSerialize<T>(this T obj) where T : class
    {
        try
        {
            return Newtonsoft.Json.JsonConvert.SerializeObject<T>(obj);
        }
        catch (Exception e)
        {
            return null;
        }
    }
}

You can use the extensions instead of ToString approach
// EXAMPLE :

ProcessValue = GetHeader(headers, PROCESS_DATA); // LINE A

if (!string.IsNullOrWhiteSpace(ProcessValue))
{
    ProcCustomer = ProcessValue.DeserializeJson<ProcCustomer>();

    if (ProcCustomer != null && !string.IsNullOrWhiteSpace(ProcCustomer.ClientId))
    {
        ProcCustomer.ClientId = HttpUtility.HtmlEncode(ProcCustomer.ClientId);

        ProcessValue = ProcCustomer.JsonSerialize();
        // or do it directly in the httpClient instead of ProcCustomer.ToString()

    }
}


JSON fetcher for Eclipse plugin This is a client module of an Eclipse plugin. I am planning to use this code as a "good exception handling" code example in one of my papers. How does it look?

HttpURLConnection httpconn=null;
BufferedReader breader=null;
try{
    URL url=new URL(this.web_service_url);
    httpconn=(HttpURLConnection)url.openConnection();
    httpconn.setRequestMethod("GET");
    System.out.println(httpconn.getResponseMessage());
    if(httpconn.getResponseCode()==HttpURLConnection.HTTP_OK)
    {
        breader=new BufferedReader(new InputStreamReader(httpconn.getInputStream()));
        String line=null;
        while((line=breader.readLine())!=null)
        {
            jsonResponse+=line;
        }
        //System.out.println(jsonResponse);
        //display_json_results(jsonResponse);
    }
}catch(MalformedURLException e){
    MessageDialog.openError(Display.getDefault().getShells()[0], "Invalid URL", e.getMessage());
}catch(ProtocolException e){
    MessageDialog.openError(Display.getDefault().getShells()[0], "Invalid Protocol", e.getMessage());
}
catch(IOException e2){
    Log.info("Failed to access the data"+e2.getMessage());
}
finally{
    try{
    breader.close();}catch(IOException e){
        Log.info("Failed to release resources"+e.getMessage());
    }

}

General
As example code, there is a fair amount to comment on. For an eclipse plugin, I would at least expect you to select-all and Ctrl-Shift-F ....

consistent use of braces (on the end of the line, not start of the new line)
consistent spacing between values and operators jsonResponse+=line; to jsonResponse += line;

The formatted code looks like:

    HttpURLConnection httpconn = null;
    BufferedReader breader = null;
    try {
        URL url = new URL(this.web_service_url);
        httpconn = (HttpURLConnection) url.openConnection();
        httpconn.setRequestMethod("GET");
        System.out.println(httpconn.getResponseMessage());
        if (httpconn.getResponseCode() == HttpURLConnection.HTTP_OK) {
            breader = new BufferedReader(new InputStreamReader(
                    httpconn.getInputStream()));
            String line = null;
            while ((line = breader.readLine()) != null) {
                jsonResponse += line;
            }
            // System.out.println(jsonResponse);
            // display_json_results(jsonResponse);
        }
    } catch (MalformedURLException e) {
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid URL", e.getMessage());
    } catch (ProtocolException e) {
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid Protocol", e.getMessage());
    } catch (IOException e2) {
        Log.info("Failed to access the data" + e2.getMessage());
    } finally {
        try {
            breader.close();
        } catch (IOException e) {
            Log.info("Failed to release resources" + e.getMessage());
        }

    }

Working with the formatted code now:

Why do you have an active System.out.println(httpconn.getResponseMessage()); in the code? That should be commented out. Use the Log for that.
Why are the HttpURLConnection and BufferedReader declared outside the try-block? There is no need.
The BufferedReader should be opened with a try-with-resource block to perform the auto-close.
you are losing newlines on the BufferedReader's readLine(). You should use a different system, or alternatively add the newline back in... unless you are using some other mechanism to reformat thte JSON.
you should be appending to a StringBuilder, not doing String concatenation (jsonResonse += line; == BAD)
why have commented-out code in example code? Get rid of the println's and the display_json_results

Exception Handling
This exception-handling has a lot of problems I can see.

you are throwing away all stack traces.... you do not report them! Why?

you have different forms of exception naming in your handlers. Two of them call the exception e. The third is called e2. Use meaningful names, or use consistent names. The mix is.... mixed up.

Calling Log.info("Failed to access the data" + e2.getMessage()); is ... poor. If you have the Log available, it should at least be a warning! Also, you should pass the full exception to the Log, and log the full trace. Finally, you do not have a space between the 'data' and the e2.getMessage() in the output... .... access the data" should be ... access the data: "

When there is an exception, you should help the user by indicating what data was causing the exception. In this case, the errors/dialogs should contain this.web_service_url since that was the source of the problem.

Since you have the log, not only should you be outputting the exception message for the URL formats to the Display, but also to the Log.

Quick reformat
I messed with the code, and got the following:
  try {
        URL url = new URL(this.web_service_url);
        HttpURLConnection httpconn = (HttpURLConnection) url.openConnection();
        httpconn.setRequestMethod("GET");
        //System.out.println(httpconn.getResponseMessage());
        if (httpconn.getResponseCode() == HttpURLConnection.HTTP_OK) {
            try (BufferedReader breader = new BufferedReader(new InputStreamReader(
                    httpconn.getInputStream()));) {
                String line = null;
                while ((line = breader.readLine()) != null) {
                    jsonResponse.append(line).append("\n");
                }
            }
            // System.out.println(jsonResponse);
            // display_json_results(jsonResponse);
        }
    } catch (MalformedURLException mue) {
        Log.warn("Invalid URL " + this.web_service_url, mue);
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid URL " + this.web_service_url, mue.getMessage());
    } catch (ProtocolException pe) {
        Log.warn("Protocol Exception " + this.web_service_url, pe);
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid Protocol " + this.web_service_url, pe.getMessage());
    } catch (IOException ioe) {
        Log.warn("Failed to access the data " + this.web_service_url, ioe);
    }

}


Merging K Sorted Arrays My aim is to merge k different sorted array with unique elements and I want a code review about it, here is my code below which is written in java;

public class MergeDemo {

    public static void main(String[] args) {
        int[] arr1 = { 1, 2, 6, 7, 18, 19 };
        int[] arr2 = { 3, 4, 5, 8, 9 };
        int[] arr3 = { 10, 11, 12 };
        int[] arr4 = { 13, 14, 15, 20};
        int[] arr5 = { 16, 17};

        int[][] arrays = { arr1, arr2, arr3, arr4, arr5 };

        handleAnswer(arrays);
    }

    public static void handleAnswer(int[][] arrays) {
        System.out.println("List of arrays to be merged;");
        System.out.println("****************************");
        for(int i = 0; i < arrays.length; i++) {
            System.out.printf("Array [%d]: ", i);
            printArray(arrays[i]);
        }

        System.out.println("");
        System.out.println("Merged array;");
        System.out.println("*************");
        printArray(merge(arrays));
    }

    public static void printArray(int[] array) {
        for(int i = 0; i < array.length; i++)
            System.out.printf("%d ", array[i]);
        System.out.print('\n');
    }

    /*
     * Merges k sorted arrays with arbitrary lengths
     * Calls overloaded merge method with two parameters
     */
    private static int[] merge(int[][] arrays) {
        int[] mergeArray = new int[arrays[0].length];
        System.arraycopy(arrays[0], 0, mergeArray, 0, arrays[0].length);

        for(int i = 1; i < arrays.length; i++) {
            mergeArray = merge(mergeArray, arrays[i]);
        }

        return mergeArray;
    }

    /*
     * Merges two sorted arrays assuming
     * they have unique elements
     */
    private static int[] merge(int[] arr1, int[] arr2) {
        int left = 0;
        int right = 0;

        int[] mergeArr = new int[ arr1.length + arr2.length ];
        int index = 0;

        while(index != mergeArr.length){
            if(left == arr1.length) {
                while(right != arr2.length) {
                    mergeArr[index++] = arr2[right++];
                }
            } else if(right == arr2.length) {
                while(left != arr1.length) {
                    mergeArr[index++] = arr1[left++];
                }
            } else {
                if(arr1[left] < arr2[right]) {
                    mergeArr[index++] = arr1[left++];
                } else {
                    mergeArr[index++] = arr2[right++];
                }
            }
        }

        return mergeArr;
    }
}

And sample output of the code above;

List of arrays to be merged;
****************************
Array [0]: 1 2 6 7 18 19
Array [1]: 3 4 5 8 9
Array [2]: 10 11 12
Array [3]: 13 14 15 20
Array [4]: 16 17

Merged array;
*************
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20

No need to copy first array

Right now, you have this code that copies the first array into mergeArray, and then you start the merging loop:

    int[] mergeArray = new int[arrays[0].length];
    System.arraycopy(arrays[0], 0, mergeArray, 0, arrays[0].length);

    for(int i = 1; i < arrays.length; i++) {
        mergeArray = merge(mergeArray, arrays[i]);
    }

You don't actually need to do this copy.  Instead, you can just do this:

    int[] mergeArray = arrays[0];

    for(int i = 1; i < arrays.length; i++) {
        mergeArray = merge(mergeArray, arrays[i]);
    }

Reduce bounds checking

Your merging function is correct, but it checks both the left and right bounds on each iteration:

    while(index != mergeArr.length){
        if(left == arr1.length) {
            while(right != arr2.length) {
                mergeArr[index++] = arr2[right++];
            }
        } else if(right == arr2.length) {
            while(left != arr1.length) {
                mergeArr[index++] = arr1[left++];
            }
        } else {
            if(arr1[left] < arr2[right]) {
                mergeArr[index++] = arr1[left++];
            } else {
                mergeArr[index++] = arr2[right++];
            }
        }
    }

It would be slightly faster to only checking the bound that was moved:

    while (true) {
        if (arr1[left] < arr2[right]) {
            mergeArr[index++] = arr1[left++];
            if (left == arr1.length) {
                while (right != arr2.length) {
                    mergeArr[index++] = arr2[right++];
                }
                break;
            }
        } else {
            mergeArr[index++] = arr2[right++];
            if (right == arr2.length) {
                while (left != arr1.length) {
                    mergeArr[index++] = arr1[left++];
                }
                break;
            }
        }
    }

If you make this change, you also need to add a check before the loop that handles the case where one or both of the arrays had length 0.


Finding the biggest product of two integers formed by the digits 1-9 The question was, which is the biggest product you can make from the digits of 1 to 9 (which is 9642*87531, BTW). It is pretty clear that one number starts with 9 and the other with 8, and that you need to append the remaining digits in descending order to one or the other number.

buildList x y [] acc = (x,y):acc
buildList x y (z:zs) acc = buildList (10*x+z) y zs (buildList x (10*y + z) zs acc)

list = buildList 9 8 [7,6,5,4,3,2,1] []

Sorting it by descending products, and taking the beginning of the list:

import Data.List
import Data.Function
result = take 10 $ sortBy (compare `on` (negate.uncurry (*))) list

I'm not satisfied with my solution for buildList, which uses explicit recursion. I'd like to have something like a fold, just not linear but with different "branches".

Here is a quick try to implement your algorithm using 'foldr'. I did not check in depth whether my solution is correct.

I have divided the problem into parts. First we define a function that takes a list and splits the list into pairs of partitions where the order of elements is preserved.

split :: [a] -> [([a],[a])]
split []     = [([],[])]
split (x:xs) = concatMap (\(fst,snd) -> [(x:fst,snd), (fst,x:snd)]) (split xs)

Employing this definition we can define the list of tuples of decimals.

combinations :: [([Int],[Int])]
combinations = map (\(fst,snd) -> (9:fst,8:snd)) (split [7,6..1])

To transform these lists of decimals into numbers we use the following transformation.

toDeci :: [Int] -> Int
toDeci = foldl (\acc x -> x + 10 * acc) 0

Combining these functions we can redefine list as follows.

list :: [(Int,Int)]
list = map (\(fst,snd) -> (toDeci fst,toDeci snd)) combinations

Now we can define split by using foldr.

We can probably improve the code by using predefined functions like the arrow function (***).

Edit: You can generalize split by using the list monad. First we defined split using foldr as follows.

foldr (\x -> concatMap (\(fst,snd) -> [(x:fst,snd), (fst,x:snd)])) [([],[])]

Now we can observe that the neutral element is return ([],[]) in the list monad and concatMap is the implementation of (>>=). That is, we can define split as follows.

foldr (\x xs -> xs >>= (\(fst,snd) -> [(x:fst,snd), (fst,x:snd)])) (return ([],[]))

This implementation can be shortened by using the predefined function foldM from Control.Monad.

foldM (\(fst,snd) x -> [(x:fst,snd), (fst,x:snd)]) ([],[])


Function to calculate energy for wind turbines This function is going to be part of a class which can determine the efficiency and max/average power that can be produced by specific wind turbines based on the location given by the user.  Is the documentation OK?

def WindEnergy_calc(density, surfaceArea, windspeed, powerCoefficient):

    """
    This function returns the value of wind energy given density of the air,

    surface area of blades, and windspeed.

    Args:

    * density (float or int) - the value represents the density of the air

    * surface area (float or int) - the value represents the total surface area of turbine

    * windspeed (float or int) - the value represents the windspeed in the area

    *Power Coeffecient of turbine (float) - value cannot exceed

    Returns:

    * total possible kinetic energy created by the wind on the turbine

    * P = 1/2*row*A*V^3*C

    """
    try:
        if powerCoefficient > 0.59:
            powerCoefficient = "Not Possible" #to break code
    except:
        print "Power Coefficient must be less than 0.59 due to Betz limit"
        return()

    half = 0.5
    windEnergy = float(half*density*surfaceArea*windspeed**3)
    return(windEnergy)

First, get rid of the double-spacing. I don't know if that's an artifact of posting on this site, or if you really coded it that way, but fix it!

def WindEnergy_calc(density, surfaceArea, windspeed, powerCoefficient):
    """
    This function returns the value of wind energy given density of the air,
    surface area of blades, and windspeed.

    Args:
    * density (float or int) - the value represents the density of the air
    * surface area (float or int) - the value represents the total surface area of turbine
    * windspeed (float or int) - the value represents the windspeed in the area
    *Power Coeffecient of turbine (float) - value cannot exceed

    Returns:
    * total possible kinetic energy created by the wind on the turbine
    * P = 1/2*row*A*V^3*C
    """
    try:
        if powerCoefficient > 0.59:
            powerCoefficient = "Not Possible" #to break code
    except:
        print "Power Coefficient must be less than 0.59 due to Betz limit"
        return()

    half = 0.5
    windEnergy = float(half*density*surfaceArea*windspeed**3)
    return(windEnergy)

Next, fix the name of your function. This is a function not a procedure. You aren't performing some "helper" operation and leaving your results in global variables or class members. You are performing a computation and returning the result! Pick an appropriate name:

def wind_energy():
def calculate_wind_energy():
def wind_energy_of():

In general, function names should include a verb and some supporting words - adverbs or nouns - to flesh out what is being done. For a "pure" function, it's possible that the verb is implicit: the word "compute" or "calculate". For example sqrt() and cos() have no verb, because the verb form would implicitly be compute_sqrt() or calculate_cosine(), which doesn't add much value. (On the other hand, we fopen() and fread() because the verb isn't quite so obvious.)
For a function taking lots of inputs, you want to have a really obvious ordering of the inputs. That is, open('foo.txt', 'r') should make it obvious that the filename is first, and the mode is second. (It doesn't, btw. But it should!)

In your case, you have four incoming parameters. None of them seem obvious to me. I'd suggest that you take advantage of python's named-only parameters to put a name on each one of them:

def wind_energy(*, density, area, velocity, power):

Above, the * marks the end of positional parameters - meaning there are none. Every parameter after the * is required to be named. So you call this like:

we = wind_energy(density=1.0, area=4.6, velocity=20, power=0.1)

Cite your sources! I found a website that I think contains your source equations:

http://formulas.tutorvista.com/physics/wind-energy-formula.html

You need to include this, and the name(s) of your equations, in your doc comment. Don't write this:

# No!
"""...returns the value of wind energy ... """

Instead, write this:

# Yes!
"""... return the wind energy computed with Farnsworth's Formula
(http://www.farnsworth.com/wind-energy-formula.html) ... """

Command your minions. This is purely a "social convention." But the standard convention for commit comments and function documentation is to use the Imperative Mood rather than passive voice. It's almost as if your program is telling the computer what to do. (Wait, what?)

Change this:

This function returns the value of wind energy...

Into this:

Return the value of wind energy ...

Likewise, when you commit your changes to Mercurial, don't write this:

I updated the code based on suggestions from codereview...

Instead, write this:

Make changes from codereview ...

Validate and rename!

You have two constraints, one explicit (the Betz limit) and the other implicit (all the parameters are numeric). Validate them!

I'll suggest you have a look at Python's numbers module, which provides abstract base classes for numeric types, and applies a "hierarchy" to them. The most appropriate type, since you use "float or int" a lot, seems to be Real. The Real type adds "conversion to float" which is right up our alley:

rho = float(density)
V = float(velocity)

(Also worth noting: rho is the name of a Greek letter. row is something you do in a boat.)

You might even consider the fact that Python supports unicode identifiers, like so:

 = float(density)

(Sadly, there's no good way to spell V³ except for V ** 3.)

For your explicit validation, you have totally bungled your code:

    try:
        if powerCoefficient > 0.59:
            powerCoefficient = "Not Possible" #to break code
    except:
        print "Power Coefficient must be less than 0.59 due to Betz limit"
        return()

First, try: / except: is used when you believe that the code inside the try: block will raise an exception. You have an if-statement inside the try block that will not raise.

Second, this is not a function that should ever call print. I understand you might insert print statements for debugging, but printing anything is outside the scope of this function. If your caller has passed you an invalid value, the correct behavior is to raise an exception - typically a ValueError - and let the caller deal with their own stupidity. (But make sure you document the limit in your doc comment!)

Try this, instead:

# Somewhere global, like outside the function or top of module:
BETZS_COEFFICIENT = 0.593
"""Betz's coefficient- see https://en.wikipedia.org/wiki/Betz's_law"""

# Inside your function:
if power > BETZS_COEFFICIENT:
    raise ValueError("Power coefficient must be <= Betz's coeff. It's the law!")

Also worth noting: when you invoke return() with empty parens, it doesn't mean "return nothing". It means "return a tuple with zero items". The () expression (note: expression) is an empty-tuple constructor. This is consistent with calling a function with no args, since positional args are a tuple:

>>> def f():
>>>     return()
...
...
>>> x = f()
>>> print(x)
()
>>> type(x)
<class 'tuple'>

What you meant to do was just a bare return. Except that was wrong, and what you really meant to do was raise, as above.


Selecting filter checkboxes using Selenium I am using Selenium Webdriver via Java. My method clicks on a filter check box (eg. make, model, etc.), that is passed in, from a modal.

When running my method in my test class, via ChromeDriver, it seems to go pretty slow and rightfully so since I am making a lot of tiny network calls for getText(). How can I optimize its performance?

public void selectFilter(String filter) {
        List<WebElement> filters = driver.findElements(selector.someWebElement());
        Map<String, WebElement> mapFilters = new HashMap();
        for (int i = 0; i < filters.size(); i++) {
            String key = filters.get(i).getText();
            WebElement value = filters.get(i).findElement(By.cssSelector("a"));
            mapFilters.put(key, value);
        }
        mapFilters.get(filter).click();
    }

The method appears to have some pointless operations.

You loop over a List<WebElement>, to build a Map<String, WebElement>, where the key of the map is the text in a WebElement in the list.
But you use only one key in the map, the String filter parameter received by the method. As such, you don't need a map at all.

This implementation is (almost) equivalent:

public void selectFilter(String filter) {
    List<WebElement> filters = driver.findElements(selector.someWebElement());
    WebElement value = null;
    for (int i = 0; i < filters.size(); i++) {
        String key = filters.get(i).getText();
        if (key.equals(filter)) {
            value = filters.get(i).findElement(By.cssSelector("a"));
        }
    }
    if (value != null) {
        value.click();
    }
}

I say almost equivalent, because I added a missed null-check.

To further improve the performance, it would be better to refine this call:

    List<WebElement> filters = driver.findElements(selector.someWebElement());

As apparently it fetches many elements that don't match the String filter parameter of the method.


Eye tracking calibration I am a newbie in std::thread and std::condition_variable.
I would like to use 2 threads: one for display (void showCalibration()) and the other for processing (void CaptureFrame()). These threads access a global variable (temp_pupil).
The display thread will clear the variable after a specific time to get the data (pupil) from processing thread (during that time).
I used loops to "simulate" capture loop from a high speed camera (void CaptureFrame()) and 9 calibration points displayed on the screen (void showCalibration()
#include <opencv2/core/core.hpp>
#include <iostream>
#include <thread>
#include <mutex>
std::vector<cv::Point2d> temp_pupil;
std::mutex mu;
std::condition_variable cond;

void showCalibration()
{
    for (int i = 0; i <= 9; i++)//plot 9 points sequentially
    {

        std::unique_lock<std::mutex> lk(mu);
        {
            temp_pupil.clear();//prepare for get new data
        }
        lk.unlock();
        cond.notify_one();//signal CaptureFrame() to capture data
        std::this_thread::sleep_for(std::chrono::milliseconds(100));//specific time

        lk.lock();
        {
            while (temp_pupil.empty())
            {
                cond.wait(lk);//wait CaptureFrame() to end current point
            }
            std::string cal = std::to_string(temp_pupil.size()) + "Cali" + std::to_string(i) + "\n";
            std::cout << cal;//print
        }
        lk.unlock();
    }
}

void CaptureFrame()
{
    cv::Point2d pupil;
    for (int i = 0; i <= 500; i++)
    {
        std::unique_lock<std::mutex> lk(mu);
        {
            pupil = cv::Point2d(i, i);//create data
            temp_pupil.push_back(pupil);//get data
            std::string cap = std::to_string(temp_pupil.size()) + "Captue" + std::to_string(i) + "\n";
            std::cout << cap;//print
        }
        lk.unlock();
        cond.notify_one();//signal showCalibration() to end current point
    }
}

int main()
{
    std::thread cal_thread(showCalibration);//display thread
    std::thread cap_thread(CaptureFrame);//process thread
    cal_thread.join();
    cap_thread.join();
}

Could anyone help me review above code ?

Design review
This code is symptomatic of a lot of the problems I see in concurrent code these days:

There is just too much synchronization cruft. Just take a look at showCalibration(): there are roughly 10 functional lines, and 6–8 of them are dealing with synchronization.
There is an over-reliance on fancy synchronization. Quite often I see code that uses mutexes, condition variables, futures, semaphores, and more, when literally a single atomic flag could do the same job. (Condition variables in particular seem to be all the hype these days; everybody is using them when there’s no real need.)
The entire program is designed around the synchronization, rather than around what it’s actually supposed to be doing. The actual algorithm is an afterthought. (Quite literally in this case.)

Good concurrent code doesn’t look like synchronization spaghetti, with the actual algorithm buried in a mess of locks and waits. Good concurrent code looks more or less the same as non-concurrent code. If you can’t figure out the actual algorithm being performed because it’s obscured by synchronization mess, then you are not writing good concurrent code.
Take a step back and look at what you’re actually trying to do. You have two threads, one producing values (pupil points), and one consuming them. Ignoring all synchronization (and stop tokens, etc.), your producer thread is basically just:
auto producer_thread()
{
    while (not done)
    {
        auto point = /* figure out point somehow */;

        points.push(std::move(point));
    }
}

And your consumer thread is basically just:
auto consumer_thread()
{
    while (not done)
    {
        if (auto const point = points.try_pop(); point)
        {
            // use *point somehow
        }
        else
        {
            // there are no points ready, so just give up our time (unless
            // there is some other computation we could do while waiting)
            std::this_thread::yield();
        }
    }
}

That’s it. That’s all you need.
Notice there are no visible locks, or condition variables, or any other such garbage. There is nothing in the code that isn’t clearly for the purpose of getting new points, or using them. (Well, except for that else block in the consumer thread, and that’s optional, though it makes a huge perfomance difference because your consumer loop won’t be wasting time spinning when there are no points available. And you could just do a pop() rather than try_pop(), and just block and wait for points instead.)
So is it actually possible to write the ideal code shown above, and still get highly performant and correct concurrency? Yes, yes indeed, and the key is right there in the code. Mutexes, locks, and condition variables are the wrong abstraction. What you want is a concurrent queue.
Now there is no concurrent queue in the standard library (yet! there is one proposed!), but you could write your own (bad idea), or use an existing one—there are several excellent implementations in Boost, TBB, and more.
I am going to make a very simple concurrent queue just for illustration. DO NOT DO THIS. Use a properly-written concurrent queue, do not roll your own. The one below will “work”, but it is terribly non-performant. It is just for illustration:
template <typename T, typename Allocator = std::allocator<T>>
class concurrent_queue
{
    std::deque<T, Allocator>    _data;
    std::mutex                  _mutex;

public:
    concurrent_queue() noexcept = default;
    concurrent_queue(Allocator const& a) : _data(a) {}

    auto push(T t)
    {
        auto lock = std::scoped_lock(_mutex);
        _data.push_back(std::move(t));
    }

    auto try_pop()
    {
        auto result = std::optional<T>{};

        auto lock = std::scoped_lock(_mutex);
        if (not _data.empty())
        {
            result = std::move(_data.front());
            _data.pop_front();
        }

        return result;
    }
};

And now your whole program basically just becomes this:
// define these to do what you actually want
auto CaptureFrame() -> cv::Point2d;
auto showCalibration(cv::Point2d) -> void;

auto producer_thread(std::stop_token stop_token, concurrent_queue<cv::Point2d>& points)
{
    while (not stop_token.stop_requested())
    {
        auto point = CaptureFrame();

        points.push(std::move(point));
    }
}

auto consumer_thread(std::stop_token stop_token, concurrent_queue<cv::Point2d>& points)
{
    while (not stop_token.stop_requested())
    {
        if (auto const point = points.try_pop(); point)
        {
            showCalibration(*point);
        }
        else
        {
            std::this_thread::yield();
        }
    }
}

auto main() -> int
{
    auto points = concurrent_queue<cv::Point2d>{};

    std::jthread cal_thread{consumer_thread, std::ref(points)};
    std::jthread cap_thread{producer_thread, std::ref(points)};

    // automatic clean shutdown and cleanup
}

This is just the most basic structure. Rather than getting a single point in the consumer thread, you could also try to pump the queue to get a whole bunch of points in a single go, then process them all at once. But with a really well designed concurrent queue—especially one with a non-blocking interface—there’s really no need for that.
Also, note that I’ve used C++20 jthreads and stop_tokens. If you can’t use C++20, no big deal. Just replace the stop_tokens with an atomic<bool>, and pass a reference to it in the thread constructors, then replace the jthreads with regular threads and manually set the atomic<bool> to signal shutdown, and manually join the threads:
auto producer_thread(std::atomic<bool>& done, concurrent_queue<cv::Point2d>& points)
{
    while (not done) // maybe using acquire or relaxed semantics
    {
        // ... [snip] ...
    }
}

auto consumer_thread(std::atomic<bool>& done, concurrent_queue<cv::Point2d>& points)
{
    while (not done) // maybe using acquire or relaxed semantics
    {
        // ... [snip] ...
    }
}

auto main() -> int
{
    auto points = concurrent_queue<cv::Point2d>{};

    auto done = std::atomic<bool>{false};
    std::thread cal_thread{consumer_thread, std::ref(done), std::ref(points)};
    std::thread cap_thread{producer_thread, std::ref(done), std::ref(points)};

    done = true; // maybe using release or relaxed semantics
    cal_thread.join();
    cap_thread.join();
}

Same diff, just more manual work.
HOWEVER!
Even that might be too much for this job!
The point of a concurrent queue is that one (or more than one!) thread can produce values while another (or several!) use those values… and—and this is the key point—none of those values are lost. But! If you are processing a video feed, and the camera frames are being generated at, say, 25 per second, while you can only process 10 per second… then usually it’s okay for you to just drop a few frames here or there. So you might not even need a queue at all. You might be able to get away with a single shared value. Something like this:
auto producer_thread(std::mutex& mutex, std::size_t& sequence_number, cv::Point2d& point)
{
    while (not done)
    {
        auto p = /* figure out point somehow */;

        auto lock = std::scoped_lock(mutex);
        point = std::move(p);
        ++sequence_number;
    }
}

auto consumer_thread(std::mutex& mutex, std::size_t& sequence_number, cv::Point2d const& point)
{
    auto last_sequence_number = sequence_number;

    while (not done)
    {
        auto has_point = false;
        auto p = cv::Point2d{};

        {
            auto lock = std::scoped_lock(mutex);

            // if the sequence number is unchanged, then the point data is old
            //
            // if it has changed, then there is new point data, so copy it,
            // update the sequence number, and then release the lock right
            // away
            if (sequence_number != last_sequence_number)
            {
                last_sequence_number = sequence_number;
                p = point;
                has_point = true;
            }
        }

        if (has_point)
        {
            // use p somehow
        }
        else
        {
            std::this_thread::yield();
        }
    }
}

And that’s without any kind of optimization whatsoever. If you wanted to get clever, then you might even be able to make a really compact type that encodes the point and sequence number in just, say, 8 bytes (like, 1 byte for the sequence number (0–255), 3 bytes each for the two coordinates (0–16777215 each), with a byte to spare for something else), and squeeze all that into a single 64-bit atomic value. That will be freakin’ fast, especially if you leverage C++20 atomic wait and notify:
class your_type
{
    std::fast_uint64_t _data;

public:
    constexpr your_type() noexcept = default;

    constexpr your_type(std::size_t sequence_number, cv::Point2d point)
    {
        // compact sequence number and point into _data
    }

    explicit constexpr operator cv::Point2d() const noexcept
    {
        // extract the point
    }
};

auto producer_thread(std::atomic<your_type>& data)
{
    auto sequence_number = std::size_t{};

    while (not done)
    {
        auto point = /* figure out point somehow */;

        data = your_type{sequence_number, point}; // maybe with release
        data.notify_one();
    }
}

auto consumer_thread(std::atomic<your_type>& data)
{
    auto last_data = your_type{};

    while (not done)
    {
        data.wait(last_data); // maybe with acquire

        auto const point = cv::Point2d{data};
        // use point somehow
    }
}

Note the code above doesn’t take into account how to detect that there is no more data. But that’s trivial to add: just use that last unused byte to signal “closed”.
Code review
std::vector<cv::Point2d> temp_pupil;
std::mutex mu;
std::condition_variable cond;

Global variables are bad. There’s no reason you can’t have these as local variables in main() (for example), and then pass references to them to the thread functions.
Also mu isn’t a great name for a variable… especially a global one.
In showCalibration():
for (int i = 0; i <= 9; i++)//plot 9 points sequentially

Reeeaaaallllyyy? Did you test that it plots 9 points sequentially?
Here’s what happened when I tested it:
for (int i = 0; i <= 9; i++)//plot 9 points sequentially
    std::cout << "plotting point #" << (i + 1) << " sequentially\n";

// Output:
plotting point #1 sequentially
plotting point #2 sequentially
plotting point #3 sequentially
plotting point #4 sequentially
plotting point #5 sequentially
plotting point #6 sequentially
plotting point #7 sequentially
plotting point #8 sequentially
plotting point #9 sequentially
plotting point #10 sequentially

Hm, that’s not 9 points. 10 ≠ 9.
There are two important lessons here, a small one and a big one.
The small lesson is that the standard form for a for loops is for (auto i = 0; i < N; ++i). Note the condition: it’s i < N… not i <= N.
The big lesson is that this is why the experts say not to write naked loops. Naked loops are error-prone. They’re also unclear. You have no idea what a loop is doing without reading the body of the loop (and even then it’s sometimes not clear). A MUCH better practice is to use algorithms. For example, if you wrote an rigorously tested a repeat<N>() function, you could use that, and it would be clear and safe:
template <std::size_t N, std::invocable F>
auto repeat(F&& f)
{
    for (auto i = std::size_t(0); i != N; ++i)
        f();
}

template <std::size_t N, std::invocable<std::size_t> F>
auto repeat(F&& f)
{
    for (auto i = std::size_t(0); i != N; ++i)
        f(i);
}

void showCalibration()
{
    repeat<9>([&](auto i) // self-documenting that you just want to repeat something 9 times
    {
        std::unique_lock<std::mutex> lk(mu);
        // ... and so on ...
    });
}

Now the biggest problem in showCalibration() (and in general) is that you go a little lock-crazy. You lock too often, hold the locks for too long, and reuse locks (which is generally a terrible idea, because it makes it harder to reason about when a lock is held or not).
Take a look at the first few lines of showCalibration():
std::unique_lock<std::mutex> lk(mu);
{
    temp_pupil.clear();//prepare for get new data
}
lk.unlock();

Okaaaay… but… why not this:
{
    auto lock = std::scoped_lock(mu);
    temp_pupil.clear();
}

That’s not only one line shorter, it’s also clearer about what the lock is really for, and how long it’s being held.
Now, this line is baffling, for several reasons:
cond.notify_one();//signal CaptureFrame() to capture data

The first problem is they you notify on this condition… but CaptureFrame() doesn’t actually wait on that condition.
Nor does it make any sense that it should. If CaptureFrame() is capturing frames from a camera, it doesn’t really need the consumer thread’s permission to capture the next frame. It should just capture the next frame as soon as it can; waiting on the processing of the previous frame or whatever defeats the purpose of doing this concurrently.
The other big problem with this is that you’re abusing condition variables. A condition variable should represent a single condition. But you are using cond (which is a terrible name for a condition variable, because it leads to exactly this kind of problem) for two conditions: “ready for the next frame to be captured” and “the next frame has already been captured and is ready for processing”.
If you actually wanted to wait on both those conditions, you would need two condition variables. But of course… you don’t really want to wait for permission to capture the next frame. (And you don’t even need to wait on the condition that the next frame is ready for processing, either.)
lk.lock();
{
    while (temp_pupil.empty())
    {
        cond.wait(lk);//wait CaptureFrame() to end current point
    }
    std::string cal = std::to_string(temp_pupil.size()) + "Cali" + std::to_string(i) + "\n";
    std::cout << cal;//print
}
lk.unlock();

Okay, reusing locks is bad. But the larger problem is that your function is complex enough that it needs to reuse a lock. If your function locks, unlocks, then relocks, and unlocks… that’s usually a sign that it should be two functions.
while (temp_pupil.empty())
{
    cond.wait(lk);//wait CaptureFrame() to end current point
}

Never wait on condition variables like this. Writing a loop like this is just an invitation to confusion and bugs.
Instead, do this:
cond.wait(lk, [] { return not temp_pupil.empty(); });

This is clean, self-contained, and impossible for any future maintainer to misunderstand or fuck up.
So, overall, this block of code can become:
{
    auto lock = std::unique_lock(mu);
    cond.wait(lock, [] { return not temp_pupil.empty(); });

    std::cout << temp_pupil.size() << "Cali" << i << '\n';
}

But let’s look at the big picture in this function:
void showCalibration()
{
    repeat<9>([&](auto i)
    {
        {
            auto lock = std::scoped_lock(mu);
            temp_pupil.clear();
        }

        std::this_thread::sleep_for(std::chrono::milliseconds(100));

        {
            auto lock = std::unique_lock(mu);
            cond.wait(lock, [] { return not temp_pupil.empty(); });

            std::cout << temp_pupil.size() << "Cali" << i << '\n';
        }
    });
}

Does it really make sense for the processing thread to clear the queue, then go to sleep, then wake up and start processing data? Doesn’t it make more sense to just wake up, process the data, clear the queue when done with the data, then go back to sleep?
void showCalibration()
{
    repeat<9>([&](auto i)
    {
        auto lock = std::unique_lock(mu);
        cond.wait(lock, [] { return not temp_pupil.empty(); });

        // process the data somehow...
        std::cout << temp_pupil.size() << "Cali" << i << '\n';

        temp_pupil.clear();
    });
}

But this holds the lock way too long, so, even better would be:
void showCalibration()
{
    repeat<9>([&](auto i)
    {
        auto data = std::vector<cv::Point2d>{};

        {
            auto lock = std::unique_lock(mu);
            cond.wait(lock, [] { return not temp_pupil.empty(); });

            std::swap(data, temp_pupil);
        }

        // process the data somehow...
        std::cout << data.size() << "Cali" << i << '\n'; // you may have to synchronize std::cout
                                                         // but I know it's just being used and example, so I'll ignore it
    });
}

And if you wanted to be more efficient and reuse the allocated memory, you could move data out of the loop, and manually clear it before swapping (or at the end of the loop). (If you wanted to simulate a processing delay, you could insert the sleep operation where the processing comment is, I suppose.)
void CaptureFrame()
{
    cv::Point2d pupil;

You generally shouldn’t declare variables until you need them.
for (int i = 0; i <= 500; i++)

Is this meant to be for (int i = 0; i < 500; i++)?
std::unique_lock<std::mutex> lk(mu);
{
    // ... [snip] ...
}
lk.unlock();

There is no reason to use a unique lock, or to manually unlock here. Just do:
{
    auto lock = std::scoped_lock(mu);
    // ... [snip] ...
}

Now, take a step back, and look at the big picture in CaptureFrame(). What this function should do is:

create the data
push the data to the shared data store
notify the waiters that there is data

The lock on the shared data is only necessary for step 2.
That means:
void CaptureFrame()
{
    repeat<500>([&](auto i)
    {
        auto pupil = cv::Point2d(i, i); //create data

        std::cout << temp_pupil.size() << "Captue" << i << '\n'; // don't forget to synchronize std::cout

        {
            auto lock = std::scoped_lock(mu);
            temp_pupil.push_back(std::move(pupil));
        }

        cond.notify_one();
    });
}

The moral of the story is that concurrency and synchronization shouldn’t swallow up your entire program. Merely doing a bunch of stuff concurrently is not the point of your program: your program is (presumably) doing something actually useful. So focus on that, design your program around that functionality, and only add concurrency and synchronization sparingly, and don’t let it become the whole focus of the program.
And also, don’t look at what’s in the standard library, and then design your synchronization around that. Look at what your program needs, and then see what the standard library offers. And if it doesn’t offer something (which is very common), then look at third party libraries. The point is: figure out what your program needs and then look for the tools that will accomplish that… don’t figure out what tools you have on hand and then jigger your program around in order to use them.
And don’t bother with condition variables. They’re inefficient and difficult to use correctly. You almost never really need them.


Efficiently remove duplicate elements from the list I have a legacy code in which ids List has some duplicate value coming and this method is being called from lot of difference places as of now -
public async Task<List<T>> Execute<T>(IList<int> ids, Policy policy, Func<CancellationToken, int, Task<T>> mapperFunc) where T : class
{
    var holder = new List<Task<T>>(ids.Count);
    var removeNull = new List<T>(ids.Count);
    for (int i = 0; i < ids.Count; i++)
    {
        var id = ids[i];
        holder.Add(ProcessData(policy, ct => mapperFunc(ct, id)));
    }

    var responses = await Task.WhenAll(holder);
    for (int i = 0; i < responses.Length; i++)
    {
        var response = responses[i];
        if (response != null)
        {
            removeNull.Add(response);
        }
    }
    return removeNull;
}

I am trying to change above method such that I can remove duplicate stuff from ids list so I came up with below code which does that but I wanted to see if there is any better way we can write below code?
public async Task<List<T>> Execute<T>(IList<int> ids, Policy policy, Func<CancellationToken, int, Task<T>> mapperFunc) where T : class
{
    var noDupsList = new HashSet<int>(ids).ToList();
    var holder = new List<Task<T>>(noDupsList.Count);
    var removeNull = new List<T>(noDupsList.Count);
    for (int i = 0; i < noDupsList.Count; i++)
    {
        var id = noDupsList[i];
        holder.Add(ProcessData(policy, ct => mapperFunc(ct, id)));
    }

    var responses = await Task.WhenAll(holder);
    for (int i = 0; i < responses.Length; i++)
    {
        var response = responses[i];
        if (response != null)
        {
            removeNull.Add(response);
        }
    }
    return removeNull;
}

Update
Seeing the answer that came I thought it's good idea to mention how my actual code is - This is how my original code is :
public async Task<List<T>> Execute<T>(IList<int> ids, Policy policy, Func<CancellationToken, int, Task<T>> mapperFunc, string logMessage) where T : class
{
    var noDupsList = new HashSet<int>(ids).ToList();
    var holder = new List<Task<T>>(noDupsList.Count);
    var removeNull = new List<T>(noDupsList.Count);
    using (var logMetric = new LogMetric(_logger, TITLE, "DatabaseCall"))
    {
        logMetric.Message = logMessage;
        for (int i = 0; i < noDupsList.Count; i++)
        {
            var id = noDupsList[i];
            holder.Add(ProcessData(policy, ct => mapperFunc(ct, id)));
        }

        var responses = await Task.WhenAll(holder);
        for (int i = 0; i < responses.Length; i++)
        {
            var response = responses[i];
            if (response != null)
            {
                removeNull.Add(response);
            }
        }
        logMetric.StatusCode = (removeNull.Count == 0) ? 204 : 200;
    }
    return removeNull;
}

I think you used a good way to filter out the duplicates, but I don't see why you feel the need to call .ToList() after that. A HashSet is perfectly iterable.
As such, I believe I would write your lengthy code somewhat differently as:
public async Task<IReadonlyList<T>> Execute<T>(IEnumerable<int> ids, Policy policy, Func<CancellationToken, int, Task<T>> mapperFunc) where T : class
{
    return (await Task.WhenAll(
            new HashSet<int>(ids).Select( id => ProcessData( policy, ct => mapperFunc( ct, id ) ) ) ) )
        .Where( response => response != null )
        .ToList();
}

It will still remove all the duplicates, await all entries using the Task.WhenAll and remove the null values.
As for knowing if it is more efficient I can't tell without testing. I think it is more readable, though to be fair, I would probably rather assign the results to a variable before filtering them out, saving you some brackets.
public async Task<IReadonlyList<T>> Execute<T>(IEnumerable<int> ids, Policy policy, Func<CancellationToken, int, Task<T>> mapperFunc) where T : class
{
    var responses = await Task.WhenAll(
            new HashSet<int>(ids).Select( id => ProcessData( policy, ct => mapperFunc( ct, id ) ) ) );

    return responses
        .Where( response => response != null )
        .ToList();
}


Copying and filtering Ruby arrays I'm doing a Ruby exercise to create new arrays with a given filter.

  Copy the values less than 4 in the array stored in the source variable
  into the array in the destination variable.

The editorial solution code is:

def array_copy(source)
  destination = []
  for number in source
    # Add number to destination if number
    # is less than 4
    destination << number if number < 4
  end
  return destination
end

But, as a beginner, I came up with this solution:

def array_copy(source)
  return source.select {|i| i < 4}
end

Is there any problems with my solution?

I think your answer is perfectly valid and much better than the editorial solution.

I only have two suggestions to improve it further:

variable name i is commonly used for array indexes. Using it instead for array values can confuse the reader of the code.
return is not needed - Ruby functions will implicitly return the value of the last evaluated expression.


Normalized random position generator I place various divs on screen by adding random x and y positions successively normalized with this function. My question is, how well is it written? Can I improve it? If so, then how?

I am worried not only about the coding style but also performance since I'm using multiple Math.random()s.

   function normalize(n) {
        var x = 0, i = 0;
        while (i < n) {
            x += Math.random();
            i++;
        }
        return (x / n);
    }

With totally zero context of what's going on and I'm not a math person, why "normalize"? What does "normalize" mean? As a front-end developer, normalize.css comes into mind, but it's not related to CSS. So I'm clueless. If you can give a better name, that would be great.

Also, a more compact way to write this is using a for loop

function normalize(n) {
  for(var x = 0, i = 0; i < n; i++){
    x += Math.random();
  }
  return (x / n);
}


Comparing multiple arguments and returns the smallest argument I want to see if anyone knows how to optimize the code below:

public class smallestTest
{
   public static void main(String[] args)
   {
      double resultSmallest = smallest(2.5, 3.5, 4.5);
      System.out.println("The smallest number out of 2.5, 3.5, and 4.5 is: " + resultSmallest);
   }

  /**
  Computes the smallest of three variables
  @param x, y, z
  @returns smallestNumber
 */

    public static double smallest(double x, double y, double z)
    {
         double smallestNumber = 0;
         if ( x < y && x < z)
         {
           smallestNumber = x;

         }
         else if (y < x && y < z)
         {
            smallestNumber = y;

         }
         else if (z < x && z < y)
         {
            smallestNumber = z;

         }

         return smallestNumber;
      }
}

You can use something called Varargs that was introduced in java 5.

It is used in method declarations to declare arguments as an array.

public void myMethod(double... doubleArray) {
}

public void callMyMethod() {
    myMethod(1.2, 4, 2, 1); // all those are put into a double[]
}

You can adapt this to your program by doing the following:

public static double smallest(double... vals) {
    // must pass at least 1 argument. You could also return Double.NaN or something
    if(vals.length == 0) throw new IllegalArgumentException("smallest must be passed at least 1 value.");
    // start with the first value as the smallest
    double smallest = vals[0];
    for(int i=1;i<vals.length;i++) {
        // see if this ones smaller than our stored smallest
        if(vals[i] < smallest) {
            smallest = val[i];
        }
    }
    return smallest;
}

You can then call this with as many double values as you like.


Check if a file path matches any of the patterns in a blacklist I'm processing a list of files, and want to check the filenames against a list of regex, like:

IGNORE_FILES = [
    re.compile('^./Vendor.*'),
    re.compile('^./Pods.*'),
    …
]

def in_blacklist(file):
    return len(list(filter(lambda r: r.match(file) != None, IGNORE_FILES))) > 0

def in_whitelist(file):
    return SWIFT_FILE_REGEX.match(file) != None

def files():
    valid_files = []
    for root, dirs, files in os.walk('.'):
        for file in files:
            if in_whitelist(file) and not in_blacklist(root):
                valid_files.append("%s/%s" % (root, file))
    return valid_files

I'm looking for a smoother way to write:

len(list(filter(lambda r: r.match(root) != None, IGNORE_FILES))) == 0

I feel that it's not so easy to read, and especially with the list(..)

You can build one regex out of the list of regexes by

'|'.join(IGNORE_FILES)

(Well, before running re.compile on them...)

Then your in_blacklist function becomes as easy to write and read as in_whitelist.

I further assume that you don't want to ignore paths that start with any letter, but those starting with a literal dot (which is what os.walk('.') yields). So you need to escape those leading dots.

Also, in your files() function, you should move your in_blacklist check out of the inner loop. No need to check the same root directory multiple times.

So:

IGNORE_FILES = re.compile('|'.join([
    r'^\./Vendor.*',
    r'^\./Pods.*',
]))

def in_blacklist(file):
    return IGNORE_FILES.match(file) != None

def in_whitelist(file):
    return SWIFT_FILE_REGEX.match(file) != None

def files():
    valid_files = []
    for root, dirs, files in os.walk('.'):
        if in_blacklist(root):
            continue
        for file in files:
            if in_whitelist(file):
                valid_files.append("%s/%s" % (root, file))
    return valid_files

And for more portability, you could replace "%s/%s" % (root, file) by os.path.join(root, file), but I wouldn't insist on this too hard...


Find missing element in an array of unique elements from 0 to n The task is taken from LeetCode

  Given an array containing n distinct numbers taken from 0, 1, 2, ...,
  n, find the one that is missing from the array.

  Example 1:

Input: [3,0,1]
Output: 2

  Example 2:

Input: [9,6,4,2,3,5,7,0,1]
Output: 8

  Note:

  Your algorithm should run in linear runtime complexity. Could you
  implement it using only constant extra space complexity?

My approach is to subtract the sum from 0-n and the sum of the elements in the array. For the sum of the number from 0 to n I use the Gauss forumula: (n * (n + 1)) / 2. For the sum in the array I will have to iterate through the whole array and sum up the elements.

My solution has time complexity of \$O(n)\$ and space complexity of \$O(1)\$.

/**
 * @param {number[]} nums
 * @return {number}
 */
var missingNumber = function(nums) {
    if (nums.length === 0) return -1;
    const sumOfNums = nums.reduce((ac, x) => ac + x);
    const sumTillN = (nums.length * (nums.length + 1)) / 2;
    return sumTillN - sumOfNums;
};

I don't think there is a better than \$O(n)\$ and \$O(1)\$ to this problem.

However you can simplify the code a little (trivial) by subtracting from the expected total.

const findVal = nums => vals.reduce((t, v) => t - v, nums.length * (nums.length + 1) / 2);

Or

function findVal(nums) {
    var total = nums.length * (nums.length + 1) / 2;
    for (const v of nums) { total -= v }
    return total;
}

BTW

The extra brackets are not needed when calculate the total

(x * (x + 1)) / 2 === x * (x + 1) / 2


Router for MVC framework The class routes URLs based on the domain.com/class/method/param/.. format. It also checks the request type (GET or POST) and calls the method name GET or POST from the identified class.

class Router {
    private $_routes,
            $_path,
            $_method,
            $_found;

    public function __construct(Array $routes, $path, $method) {
        krsort($routes);

        $this->_routes = $routes;
        $this->_path   = $path;
        $this->_method = strtoupper($method);
    }

    public function load() {
        foreach($this->_routes as $regex => $class) {
            $regex = str_replace('/', '\/', $regex);
            $regex = '^' . $regex . '\/?$';

            if(preg_match('/' . $regex . '/i', $this->_path, $params)) {
                $this->_found = true;

                $this->_classInstantiate($class, $params);

                break;
            }
        }

        if(!$this->_found) {
            throw new Exception('URL location not found: ' . $this->_path);
        }
    }

    private function _classInstantiate($class, $params) {
        if(class_exists($class)) {
            $obj = new $class($params);

            if(method_exists($obj, $this->_method)) {
                $method = $this->_method;

                $obj->$method();
            } else {
                throw new BadMethodCallException('Method not found: ' . $this->_method);
            }
        } else {
            throw new Exception('Class not found: ' . $class);
        }
    }
}

I load it up in my entry file like so:

//url to class router
$routes = (Array) $config->routes;
$path   = $_SERVER['REQUEST_URI'];
$method = $_SERVER['REQUEST_METHOD'];

$router = new Router($routes, $path, $method);
$router->load();

Am I utilizing dependency injection correctly?
Are there any parts of my class that I should decouple?
Is there anything I could improve in my code when it comes to OO PHP (and MVC)?

Let's look at your questions individually:

Am I utilizing dependency injection correctly?
Yes and no. You are, essentially, injecting the routes. That's fine. But you extract them from a $config object. If you are working out some MVC (micro-) framework, then chances are that config object is an instance of a specific class.

As your code-base grows, so will the config, and chances are pretty good that the Route class will need more data from the config. Instead of passing an array to the constructor then, I'd simply pass the whole $config object, and type-hint that.
That way, when you only have to refactor the Route constructor (as opposed to all code that creates a new Route instance) if you decide to add to the config object, something the Route class needs. For example: a list of approved/registered classes that the load method can instantiate:

private $routes = null;
private $classMap = array();
public function __construct(Config $conf)
{
    $this->routes = (array) $conf->routes;
    foreach ($conf->classMap as $class => $paramData)
    {//paramData could contain which arguments are required
     //which are optional, or default values
        $this->classMap[$class] = array(
            'required' => isset($paramData['required']) ? $paramData['required'] : array(),
            'defaults' => isset($paramData['defaults']) ? $paramData['defaults'] : array()
        );
    }
}

The load method... There is a lot to say about this, but as it now stands, I have a couple of suggestions:

Why not group the $routes by method? Suppose you have 100 routes in total, 20 of which are POST routes, what good does looping through all 100 routes do?

You are not properly escaping the path regex. Suppose there is a question mark in the $regex string? By calling str_replace('/', '\/', $regex); you're also not quite doing what you thing you are doing. You're creating an escape sequence, escaping the forward slash, not the backslash you effectively need!
You want to escape the delimiter, so you should escape the backslash: use str_replace('/', '\\/', $regex);
However all this is pretty useless anyway, simply because there exists a ready-made PHP function that can escape a string that is going to be used in preg_match a lot better anyway:

 $regex = preg_quote($regex, '/');

And instead of calling this function every time the load function is called: use a setter (or 2 setters) for the $this->routes property:

public function setRoutes(array $routes)
{
    //assume GET && POST keys, too:
    $this->routes = array(
        'GET' => array(),
        'POST' => array()
    );
    foreach ($this->routes as $method => $x)
    {//ignore $x
        if (!isset($routes[$method]))
            continue;//ignore. Be weary: case sensitive, a class would be useful here
        foreach ($routes[$method] as $regex => $class)
        {
            //optionally check $class is allowed by $this->classMap!
            $this->routes[$method][preg_quote($regex, '/')] = $class;
        }
    }
}

Now this is written off the top of my head, and a bit messy. What follows is messier still, but just so you know: you can write this as a one-liner, if you use / as the regex delimiter:

$this->routes = array(
    'GET'  => array_combine(
        array_map(
            'preg_quote',
            array_keys($routes['GET'])
        ),
        $routes['GET']
    ),//same for POST
);

If you decide to use a different delimiter:

$this->routes = array(
    'GET'  => array_combine(
        array_map(
            'preg_quote',
            array_keys($routes['GET']),
            array_fill(0, count($routes['GET']), '~')//~ is now your delimiter
        ),
        $routes['GET']
    ),
);

Now, the $method and $params arguments from the constructor are only really used in the load method. Why pass them to the constructor? Why not pass them to load, and make them optional? If ever you want to write tests for this class, surely it would be easier to be able to pass invalid/impossible requests to this class.
And even when not testing: Sometimes you want your controller to perform the actions defined in 2 distinct controller actions. By changing the route, and call load again, you could do just that.

Making them optional implies that calling load will have load fall back to the $_SERVER super-global. Even so, let me now tell you why you shouldn't refactor this Route class too much, by answering your next question:

Any parts of my class that I should decouple?
Perhaps you should first ask yourself what this class is? Its name, to me, suggests it is a router class. What is a router class's job? Surely, it is to parse, validate, generate URLS, and perhaps redirect.
The URL it extracts from a request can then be used by a dispatcher or front-controller to create an instance of a specific controller, and invoke methods.

A router can indeed check to see if there are some request parameters, but processing and validating those parameters falls, in turn, under the responsibility of a Request object.
On more details on this matter, you can check this answer of mine, which reviews a Request object.

So: the answer to this question is: yes. Your code is not SOLID, as it violates the single responsibility principle by acting as a dispatcher, router, and in some ways request object factory.
Well, it doesn't function as a request object just yet, but it passes the request params to the classes it instantiates. Chances are that, in time, you'll be adding some validation methods to this class, or create a new object (Params or something), and have your Route class create those instances as it passes them to the classes load creates...

Anything I could improve in my code when it comes to OOP (and MVC)?
At this point: you're going to have to work on this some more: you show one class that is quite small. There are signs of promise, but I can't honestly assess your OO skills or correctness of your understanding of OO paradigms and MVC pattern.

The only thing I really can and will criticize on is your coding style. Now you may think this is a simple matter of personal preference, but really: it's important to follow certain standards! Such standards exist: Python has PEP, PHP has FIG, their site: PHP-FIG.
One of the things I've noticed is your tendency of prepending every non-public property/method with an underscore. This practice stems from the days where PHP didn't (yet) have access modifiers - back in the days of PHP4.
Read through the PSR documents, and conform to those standards as much as you can.
I try to conform to everything in there, except for my Allman brackets. I know, I need to practice what I preach, but a single deviation from the standards is not that bad, IMO, especially if you have a good IDE, simply re-format your code before commiting to GIT, and nobody need ever know :).

On the last, I have one thing that is rather worrying to me: You create an instance of a given class, and pass the request params to either a GET or POST method. That sounds to me like you have 1 class for each possible URL. That is absolutely not the way to go

Before you start, always see what is already out there: Symfony2, Zend, CI, Cake... are all MVC frameworks that have been around for a long time. They are all open-source, and have a routing component. Just check the source code of those projects, and see if you can't get any ideas from that

Remember: A good programmer is a lazy one. Don't invent something that already exists.


A cross platform function to get current time in seconds with precision of micro second double time_in_seconds() {
    std::chrono::time_point<std::chrono::system_clock,std::chrono::microseconds> tp = std::chrono::time_point_cast<std::chrono::microseconds>(std::chrono::high_resolution_clock::now());
    auto tmp = std::chrono::duration_cast<std::chrono::microseconds>(tp.time_since_epoch());
    std::time_t time_micro = tmp.count();
    return time_micro / 1000000.0;
}

Here C++11 chrono is used to be able run on multi platform. Use high_resolution_clock to hope to get precision of micro second. std::time_t should be enough to hold this number.

Leveraging the standard library is the best way to obtain cross-platform code, so you're on the right track. The problem is that your doesn't compile with one of the compilers I've tested it with (clang 8). Using auto would make your code simpler and avoid problematic conversions:

double time_in_seconds() {
    // now compiles with both clang 8 and gcc 9
    auto tp = std::chrono::time_point_cast<std::chrono::microseconds>(std::chrono::high_resolution_clock::now());
    auto tmp = std::chrono::duration_cast<std::chrono::microseconds>(tp.time_since_epoch());
    auto time_micro = tmp.count();
    return time_micro / 1000000.0;
}

Besides, there is no need for those intermediary, ill-named variables tp and tmp. If your function is contained in an implementation file (.cpp), where it should be, importing the std::chrono namespace is perfectly fine, and it becomes quite readable:

double time_in_seconds() {
    using namespace std::chrono;
    return duration_cast<microseconds>(system_clock::now().time_since_epoch()).count() / 1000000.0;
}

auto would also be a better choice for the return type:

auto time_in_seconds() {
    using namespace std::chrono;
    return duration_cast<microseconds>(system_clock::now().time_since_epoch()).count()
           / 1000000.0l; // long double literal
}

auto here avoids any discrepancy between the promotion done in the arithmetic operation (the division by 1.000.000) and the promotion to the return type. For instance:

long double foo() { // seemingly long double precision
    return some_integer / 100.01f; // actually float precision
}

A last improvement I think of is the use of enhanced literals:

auto time_in_seconds() {
    using namespace std::chrono;
    return duration_cast<microseconds>(system_clock::now().time_since_epoch()).count()
           / 1'000'000.0l; // you can separate thousands with '
}


Morse Code Conversion Challenge

Write a program which reads a file containing Morse Code and outputs the conversion.

Specifications

The first argument is a path to a file.
The file contains multiple lines.
Each line is a string of Morse code.

Each letter is separated by a space.
Each word is separated by two spaces.

Constraints

The input file is correctly formatted.
The Morse Code strings are alphanumeric.

Sample Input

.- ...- ..--- .-- .... .. . -.-. -..-  ....- .....
-... .... ...--

Sample Output

  AV2WHIECX 45
  BH3

Source

My Solution:

import sys
import re

morse_alphabet = {
    ''      : ' ',
    '.-'    : 'A',
    '-...'  : 'B',
    '-.-.'  : 'C',
    '-..'   : 'D',
    '.'     : 'E',
    '..-.'  : 'F',
    '--.'   : 'G',
    '....'  : 'H',
    '..'    : 'I',
    '.---'  : 'J',
    '-.-'   : 'K',
    '.-..'  : 'L',
    '--'    : 'M',
    '-.'    : 'N',
    '---'   : 'O',
    '.--.'  : 'P',
    '--.-'  : 'Q',
    '.-.'   : 'R',
    '...'   : 'S',
    '-'     : 'T',
    '..-'   : 'U',
    '...-'  : 'V',
    '.--'   : 'W',
    '-..-'  : 'X',
    '-.--'  : 'Y',
    '--..'  : 'Z',
    '-----' : '0',
    '.----' : '1',
    '..---' : '2',
    '...--' : '3',
    '....-' : '4',
    '.....' : '5',
    '-....' : '6',
    '--...' : '7',
    '---..' : '8',
    '----.' : '9'
}

def decode(morse):
    decoded = ''
    line = re.split('\s', morse)
    for letter in line:
        decoded += morse_alphabet.get(letter)
    return decoded

def main(filename):
    with open(filename) as input_file:
        for line in input_file:
            print(decode(line))

if __name__ == "__main__":
    try:
        main(sys.argv[1])
    except:
        sys.exit("No argument provided / file not found.")

Applying the insight gained from my previous question. It seems fairly succinct but I wonder is it pythonic?

My remarks are similar to last time.  morse_alphabet should be MORSE_ALPHABET.  Repeated string concatenation (decoded += …) is bad for performance.  The main task could be accomplished using a one-liner substitution.

MORSE_ALPHABET = {
    ' '     : ' ',        # Note: Changed the key from '' to ' '
    '.-'    : 'A',
    '-...'  : 'B',
    '-.-.'  : 'C',
    …
}

def decode(morse):
    return re.sub(' ?( |[.-]*)', lambda m: MORSE_ALPHABET.get(m.group(1)), morse)

The main code would be simpler if you used fileinput.  Despite the current Pokémon craze, you should not Catch 'Em All, but only catch the exceptions that you intend to handle.

def main():
    try:
        for line in fileinput.input():
            print(decode(line), end='')
    except IOError as e:
        sys.exit(e)

if __name__ == "__main__":
    main()


WordPress anti-spam script This is the first PHP script I have written. It downloads and installs a plugin called "anti-spam" by using the WordPress API. It also provides an option to either delete all comments or delete the comments, which are unapproved or marked as SPAM.

        <title>SPAM tool</title>

Simple SPAM Managing tool

<form action="" method="post">
<input type="radio" name="radio" value="all">Delete ALL comments

<input type="radio" name="radio" value="un">Delete UNapproved comments

<input type="submit" name="submit" value="Kill spam" />
</form>

            <?php

include "wp-config.php"; #adds Wordpress configuration, so we can use DB config
include "wp-load.php";
include "wp-admin/includes/plugin.php";
$connect = mysql_connect(DB_HOST, DB_USER, DB_PASSWORD); #connect to DB
    if (!$connect) {
    echo 'Database connect failed';
    } else {
    echo 'Connection to database OK!';
    }
        echo "
";
$select = mysql_select_db(DB_NAME);
$tablename = $table_prefix . "comments";
$query  = ("SELECT COUNT(comment_approved) FROM $tablename WHERE comment_approved = 0");
$execute = mysql_query($query, $connect);
    if ($execute) {
            echo ("Comments UNapproved (comment_approved = 0): "), mysql_result($execute, 0);}
    else {echo "Comment count failed";}

        echo ('
');
$query  = ("SELECT COUNT(comment_approved) FROM $tablename");
$execute = mysql_query($query, $connect);
    if ($execute) {
            echo ("Comments total: "), mysql_result($execute, 0);}
        else {echo "Comment count failed";}

        echo ('
');
    $args = (object) array( 'slug' => 'Anti-spam' );

    $request = array( 'action' => 'plugin_information', 'timeout' => 15, 'request' => serialize( $args) );

    $url = 'http://api.wordpress.org/plugins/info/1.0/';

    $response = wp_remote_post( $url, array( 'body' => $request ) );

    $plugin_info = unserialize( $response['body'] );

    $durl = $plugin_info->download_link;

    echo "$durl";
$download = file_put_contents("antispam.zip", file_get_contents("$durl")); #downloads the Anti-spam plugin
if ($download) {
echo "Download OK!";}
else {
echo "Download FAIL!";} #check if download is OK
echo "
";
$path = getcwd();
$zip = new ZipArchive; #extracts the .zip
if ($zip->open('antispam.zip') === TRUE) {
    $zip->extractTo("$path/wp-content/plugins/");
    $zip->close();
    echo 'Plugin extracted';
} else {
    echo 'Extract Failed';
    }
echo "
";
$activate = activate_plugin( "$path/wp-content/plugins/anti-spam/anti-spam.php" ); #Activate the plugin
if ( is_wp_error( $activate ) ) {

} else {
echo 'AntiSPAM plug-in activated!';}

echo ('
');
if (isset($_POST['submit'])) {
if(isset($_POST['radio']))
{
if ($_POST['radio']=="all") { $select = mysql_select_db(DB_NAME);
$tablename = $table_prefix . "comments";
$query  = ("TRUNCATE $tablename"); #TRUNCATE comments
mysql_query($query, $connect);
echo "DELETED!";
}
elseif ($_POST['radio']=="un") { $select = mysql_select_db(DB_NAME);
$tablename = $table_prefix . "comments";
$tablename2 = $table_prefix . "commentmeta"
$query  = ("DELETE FROM $tablename WHERE comment_approved = 0 or comment_approved = 'spam'");
mysql_query($query, $connect);
$query2  = ("DELETE FROM $tablename2 WHERE comment_id NOT IN (SELECT comment_id FROM $tablename)");
mysql_query($query2, $connect);
echo "DELETED!";
    } }}
else{ echo "Please choose any radio button.";}
mysql_close($connect);
exit;
?>

You can also check the project's repository at GitHub.

Coding Style and Conventions

A nice coding style can dramatically improve readability, and thanks to IDEs, it's incredibly easy to achieve.

You should use correct indentation! This would make it a lot easier to see what blocks of code belong together (this goes for the PHP and the HTML code).
You should handle the same thing the same way: Sometimes your closing curly brackets are on the same line, sometimes the next; sometimes you use echo($var), and sometimes echo $var (the second version is how it's usually done).
be consistent with your casing. SQL keywords should always be all-uppercase, not only sometimes.
you have some unnecessary parentheses (eg $query  = (...)).

Functions

Extracting functionality to functions increases readability as it's easier to see what a block of code does. It also decreases duplication and increases reusability of your code.

You could introduce functions such as:

countComments($onlyApproved): this can remove the SELECT COUNT(comment_approved) FROM $tablename duplication.
downloadPlugin($pluginName)
extractZip($fileName)
deleteComments($types): $types can be all, unapproved or spam.

Naming

Generally, your variable names are quite clear, but some of them could be more descriptive. Eg un could be unapproved, tablename could be tableNameComments, tablename2 could be tableNameCommentsMeta, query could be deleteComments and query2 could be deleteCommentsMeta.

Misc

If you have an error (eg the database connection failed), don't just continue. Return/Redirect/Die instead.
You should not use mysql_*, as it's deprecated (since 2013). Use mysqli_* or PDO instead (or use the wrapper that WordPress supplies)).
It seems that currently your code always installs the plugin, and additionally deletes comments if the user wants to, which doesn't seem to make that much sense. If I already installed the plugin, and only want to use the comment deletion option of your code, I would need to still download the plugin, which seems like a waste of resources.

WordPress

You should check out how to write plugins for WordPress, it would be a lot nicer than a plain PHP file.

For example, this would make it easier for a reader to understand how your includes work (eg wp-config.php isn't really needed, it's loaded by wp-load.php; on the other hand, you use functions from files which you don't explicitely include).

It would also add some security. Currently, it seems that anyone can access this script, not just admin users, which would be pretty bad. An attacker could delete all comments, and can cause your website to slow down significantly (because the script downloads and unzips a big file each time it is called).


Sequence counter with persistence In one of my object I needed a sequence I would use to increment an Id field just like we often do with classical databases. As I have not found a suitable solution, I wrote one. This solution uses objectify.

The first approach I came with was to have a class with a static AtomicInteger field like this:

public class MySequence {
  static AtomicInteger sequence = new AtomicInteger(0);
}

I would then just call it each time I need it.

Problem:

Every time the server is restarted, the sequence is too. So we need to persist the last value.

Second approach:

Persist the last value with objectify:

 public class MySequence {
        private static AtomicInteger sequence = new AtomicInteger(0);

        private static List<PersistSequence> l = null;

private static PersistSequence newPs = null;
    /**
     * @param user_id the id of the user we want to get the last value of the sequence
     * @return last value of PersistSequence
     */
    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type( PersistSequence.class).filter("user_id", user_id).limit(1).list();//grabs PersistSequence object bounded to user_id
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }else{
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }
        }else{
            persistCounter( user_id, sequence.incrementAndGet());
            return sequence.get();
        }
    }

    /**
     * @param user_id the id of the user
     * @param lastId last used Id
     */
    private static void persistCounter( String user_id, int lastId){
        //we only want one PersistSequence object per user so we delete the previous one before saving the new PersistSequence object
        List<Key<PersistSequence>> keys = ofy().load().type(PersistSequence.class).filter("user_id", user_id).keys().list();
        ofy().delete().keys(keys).now();
        //save the new one
        newPs = new PersistSequence();
        newPs.setLastId(lastId);
        newPs.setUserId(user_id);
        ofy().save().entities( newPs).now();
        }
    }

I am happy with this code as it works well. However, I wonder if it still will rock if there is a high volume of users calling it.

Shouldn't get id twice

I'm concerned with this code:

        persistCounter( user_id, sequence.incrementAndGet());
        return sequence.get();

After you atomically increment and get the counter, you call sequence.get() again.  That seems wrong because what happens if some other thread incremented the counter in the meantime?  I think your code should be:

        int newId = sequence.incrementAndGet();
        persistCounter(user_id, newId);
        return newId;

Repeated code

Also, I noticed that the same block of code (the one above) appears three times in a row:

    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type(PersistSequence.class).
                    filter("user_id", user_id).limit(1).list();
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }else{
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }
        }else{
            persistCounter( user_id, sequence.incrementAndGet());
            return sequence.get();
        }
    }

Since that code happens for every case, you can simplify your function by extracting that common code and placing it right before the function returns:

    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type(PersistSequence.class).
                    filter("user_id", user_id).limit(1).list();
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
            }
        }
        int newId = sequence.incrementAndGet();
        persistCounter(user_id, newId);
        return newId;
    }

Concurrency issues in persistCounter()

It's possible that persistCounter() will get called with out of order lastId arguments.  For example, it's possible that you might get called with lastId of 5 and then later get called with a lastId of 4.  So I think before you save the new counter, you should compare it with the old one to make sure that are actually saving a newer counter.

Also, there may be a concurrency problem with this function.  What happens if one thread tries to save 4 and another tries to save 5 at the same time?  Could it possibly end up saving 4 instead of 5?


Implementation of BFS on a graph I have written the code below as a BFS implementation. Could someone please review. In particular, I want to know if there could be a memory leak.

#include <iostream>
#include <list>

class Graph
{
  int number_of_vertices;
  std::list<int> *adjList;

  public:
          Graph(int number_of_vertices);
          void addEdge(int first_vertex, int second_vertex);
          void BFS(int vertex);
};

Graph::Graph(int number_of_vertices)
{
  this->number_of_vertices = number_of_vertices;
  adjList = new std::list<int>[number_of_vertices]; // Allocates space for list equal to number_of_vertices.
}

void Graph::addEdge(int first_vertex, int second_vertex)
{
  adjList[first_vertex].push_back(second_vertex);
}

void Graph::BFS(int vertex)
{

  bool *visited = new bool [number_of_vertices];

  for (int i = 0; i < number_of_vertices; i++)
  {
      visited[i] = false;
  }

  visited[vertex] = true; // Mark the vertex as visited
  std::list<int> queue;
  queue.push_back(vertex);

  while(!queue.empty())
  {
    vertex = queue.front();
    std::cout << vertex << " ";
    queue.pop_front();

    for(auto it = adjList[vertex].cbegin(); it!= adjList[vertex].cend(); ++it)
    {
      if(!visited[*it])
      {
        visited[*it] = true; // Mark the vertex as visited
        queue.push_back(*it);
      }
    }

  }
}

// Driver Program
int main()
{
    // Create a graph
    Graph g(4);
    g.addEdge(0, 1);
    g.addEdge(0, 2);
    g.addEdge(1, 2);
    g.addEdge(2, 0);
    g.addEdge(2, 3);
    g.addEdge(3, 3);

    std::cout << "Following is Breadth First Traversal "
         << "(starting from vertex 2) \n";
    g.BFS(2);

    return 0;
}

Memory

You allocate memory twice but you never deallocate it later. A good practice is to have all your new and new[] followed by a delete and delete[] respectively when you're done with the memory.

Constructor

It is a good habit to use the initialization list for your constructor. In your example, the constructor will first initialize number_of_vertices with a default value, then it'll dereference the this pointer and finally assign the value given in parameter. Using the initialization list will avoid the first two steps.
It is also better the avoid the use of this for initialization as it is not necessary and will add the cost of dereferencing the pointer.

Arrays

In general you should avoid the use of array + new as it can lead to errors (for example, you forgot to release the memory at the end).

std::vector are arrays under the hood with the same performance, you should not fear to use them even if the size of your array won't change later. In your case, you can even initialize it with the correct size and default value, avoiding the for loop:

std::vector<bool> visited(number_of_vertices, false);


Gathering response from a pagable service I'm wondering how this code could be improved. I especially don't like the use of mutable Buffer but not sure what the best way to get it out cleanly.

  def find(service: Service): Traversable[Long] = {
    def loop(lastId: Option[Long])
            (acc: mutable.Buffer[Long])
            (quota: Int): Traversable[Long] = {
      val response = service.call(lastId.getOrElse(-1))
      if (response.size() > 0 && quota > 0) {
        loop(response.last.getId)(acc ++ response)(quota - 1)
      } else {
        (acc ++ response).toList
      }
    }
    loop(None)(mutable.Buffer())(3)
  }

You might want to try Scala Streams.

val Quota = 3

def find(service: Service) = {
    def chunks(lastIdOption: Option[Long]): Stream[Stream[Int]] = {
        Stream.cons(response, chunks(response.last.id))
    }
    chunks(None).take(Quota).flatten
}

The chunks function returns an infinite stream of pages. The take(3) limits it to three pages. The flatten method joins the inner list in a large stream.

Stream is lazy, so it should fetch only the data that are needed. However, you should not expect some artificial intelligence in the Streams. For example this will always fetch the first page even if quota is zero. I hope this is not an issue in your case. If you are in doubt with the laziness, you can add some print in the chunks function to check (or falsify) your assumptions.


Positioning text and images on a site grid I've just started learning HTML and CSS. Currently I'm creating a test site to practice with.

I'm just after an early idea if the principles I'm using are correct. The below is an snippet from the main body of my HTML code. It's just about positioning text and images on a grid at the moment. I seem to be using a lot of divs with IDs in the CSS for styling etc.

Some of the alt text is work in progress and repeated currently hence why it repeats.

Can someone tell me if the structure is ok, or not and what I'd need to do to change this?

    About Us &nbsp &nbspOur Cheeses &nbsp &nbspContact Us

"QUOTE 1 - We really love the taste of your cheese, we will be back for more"

"QUOTE 2 - We really love the taste of your cheese, we will be back for more"

a) Don’t use &nbsp &nbsp to separate the navigation links. You could use ul instead (and style it with CSS display:inline; if required):

About Us

Our Cheeses

Contact Us

b) div has no meaning. You should only use div if there is no other appropriate, "narrower" element. If you are quoting something, use blockquote. If you have a navigation menu, use nav. Of course you can use div in addition to those appropriate elements, if needed.

c) This is not an error, but an opinion-based advice: If you don’t have a strong reason, don’t add id attributes just because you need a styling hook. You could use the class attribute even for what some call "unique" elements. Good reasons to use id are: in-page links (fragment identifier) and, sometimes, JavaScript hooks.