Search products from database on base of search filters I am writing a code to search Product database and return result to user.
user can select various options like product attributes that varies from product to product, also there may be case like for single attribute there can be multiple value. For Example user can select brand as filter and can select from given brand under brand option. There can be multiple tag for filter like this

Please guide me to improve this code, is there any design pattern i should use or can replace some statement with other to improve in any Sense.

public Models.Custom_Model.SubcategoryProductViewModel SubCategoryAds(long subcategoryId, Models.Custom_Model.SearchModel search)
    {
        var products = _db.products.Include(a => a.category).Include(c => c.subcategory).Include(c=>c.productattributes).Where(c => c.SubCategoryId == subcategoryId && c.subcategory.isActive && c.category.isActive);
        switch (search.criteria)
        {
            case Enum.SortingCriteria.New:
                products = products.OrderByDescending(c => c.ProductId);
                break;
            case Enum.SortingCriteria.Discount:
                products = products.OrderByDescending(c => c.Discount);
                break;
            case Enum.SortingCriteria.PricebyAsc:
                products = products.OrderBy(c => c.Price);
                break;
            case Enum.SortingCriteria.PricebyDsc:
                products = products.OrderByDescending(c => c.Price);
                break;
        }
        if(search.filter != null)
        {
            var fs = search.filter.SelectMany(c => c.Value.Select(a => new Models.Custom_Model.IdName {Id = c.Id,Name = a}));
            products.Where(z=>((z.productattributes.Select(c=>new Models.Custom_Model.IdName {Id = c.AttributeId,Name = c.Value})).Intersect(fs)).Any());
        }
        var productids = products.Skip((search.pageNo - 1) * search.pagesize).Take(search.pagesize).Select(c => c.ProductId).ToList();
        var subcategory = _db.subcategories.Include(c => c.category).SingleOrDefault(c => c.SubCategoryId == subcategoryId);
        string Name = string.Empty;
        Models.Custom_Model.IdName category = null;
        if (subcategory != null)
        {
            Name = subcategory.Name;
            category = new Models.Custom_Model.IdName() { Id = subcategory.CategoryId, Name = subcategory.category.Name };
        }
        var count = products.Count();
        var product = Business_Logic.ProductTile.ToproductTile(productids, _db);
        return new  Models.Custom_Model.SubcategoryProductViewModel {Category = category ,Name = Name,TotalItems = count, Products = product };
    }

search Model

public class SearchModel
{
    private int _pagesize = 50;

    public int pagesize
    {
        get { return _pagesize; }
        set { _pagesize =value; }
    }

    private int _pageNo = 1;

    public int pageNo
    {
        get { return _pageNo; }
        set { _pageNo = value; }
    }

    private Enum.SortingCriteria _criteria = Enum.SortingCriteria.New;

    public Enum.SortingCriteria criteria
    {
        get { return _criteria; }
        set { _criteria = value; }
    }

    private List<FiltersViewModel> _filter = null;

    public List<FiltersViewModel> filter
    {
        get { return _filter ; }
        set { _filter  = value; }
    }

}

FilterViewModel

public class FiltersViewModel
{
    public long Id { get; set; }

    public IEnumerable<string> Value { get; set; }
}

At least a couple things to note:

Bug?

I think this part doesn't work because you don't assign the result of this query back to the variable.

if(search.filter != null)
{
    var fs = search.filter.SelectMany(c => c.Value.Select(a => new Models.Custom_Model.IdName {Id = c.Id,Name = a}));
    products.Where(z=>((z.productattributes.Select(c=>new Models.Custom_Model.IdName {Id = c.AttributeId,Name = c.Value})).Intersect(fs)).Any());
}

Naming

Public properties should be in PascalCasing (msdn) private variables in camelCase.

Also, some using-statements would remove the namespace-clutter.

Line breaks

I think couple of line breaks would make long lines more readable. First line as an example.

var products = _db.products
    .Include(a => a.category)
    .Include(c => c.subcategory)
    .Include(c => c.productattributes)
    .Where(c => c.SubCategoryId == subcategoryId && c.subcategory.isActive && c.category.isActive);

Refactoring to smaller methods

Pagination (skipping and taking x records) is pretty general concept, so it should perhaps be moved to to its own, generic function.

Also, sorting could be moved to its own method

SearchModel refactoring

I remove private variables from SearchModel because they are only used for defaults. I would set defaults in constructor so defaults are in one place. Filter doesn't need to be set but I set it here for consistency.

I also renamed Criteria to SortingCriteria to make the meaning clearir. If it is only Criteria, it is not clear if it is used to filter the query or sorting.

public int PageSize { get; set; }
public int PageNumber { get; set; }
public Enum.SortingCriteria SortingCriteria { get; set; }
public List<FiltersViewModel> Filter { get; set; }

public SearchModel()
{
    PageSize = 50;
    PageNumber = 1;
    SortingCriteria = Enum.SortingCriteria.New;
    Filter = null;
}


Test file word cloud in F# This code is meant to take in a command line argument and output a 'tag cloud'. It's more of an exercise in learning F# for me because this is my first non-tutorial code file. How could this code be improved?

open System.IO
open System
[<EntryPoint>]
let main args =

    let directory = args.[0]

    let fileNames = Directory.EnumerateFiles(directory, "*Tests.cs", SearchOption.AllDirectories) |> Seq.toList

    let allLines = List.collect(fun (x:string) -> System.IO.File.ReadLines(x) |> Seq.toList) fileNames

    let allWords = List.collect(fun (x:string) -> x.Split([|' ';'_';';';':';'(';')';'\\';'/';'>';'<';'{';'}';'0';'1';'2';'3';'4';'5';'6';'7';'8';'9';'.'|], StringSplitOptions.RemoveEmptyEntries) |> Seq.toList) allLines
    //printfn "%A" allWords

    let countOccurance (word:string) list =
        let count = List.filter (fun x -> word.Equals(x)) list
        (word, count.Length)

    let distinctWords = allWords |> Seq.distinct |> Seq.toList

    let print (tup:string*int) =
        match tup with
        | (a,b) -> printfn "%A: %A" a b

    let rec wordCloud distinct (all:string list) (acc:(string*int) list) =
        match distinct with
        | [] -> acc
        | head :: tail ->
            let accumSoFar = acc @ [(countOccurance head all)]
            wordCloud tail all accumSoFar

    let acc = []
    let cloud = (wordCloud distinctWords allWords acc)

    let rec printTup (tupList:(string*int) list) =
        match tupList with
        | [] -> 0
        | head :: tail ->
            printfn "%A" head
            printTup tail

    printTup cloud

    0

inb4 Scott :-)

Use functions more. Composability is not very important in this example, but it's good to get into that habit early on.

In that spirit, turn the non-trivial value bindings into functions instead of closing over a previously defined value. Also try turning the non-trivial lambda expressions into functions; that gives them a name and makes the call site more readable.

Also, pipe more. :-) Especially for the list/sequence functions that take a higher order function, it's more readable to pipe the list into them, because it makes it more obvious what you're starting from.

Also, in my experience, defining "constant" values and functions first before actually doing any work makes the code much easier to follow, because you don't have to think about what values you already "have" at which point.

Oh, and while tail recursion is a very cool thing to have, it's not very useful here and actually makes the code quite a bit less understandable.

Did you put the type annotations in on purpose? In most cases, you don't need them because of F#'s powerful type inference. They can sometimes make code more easy to follow, but they might also constrain it where it could actually be more generic, and in general not having to specify types all the time is one of the really nice things about F#. :-)

Edit: Reading this again made me think of another important point: The individual function declarations don't belong in the main function; moving them out of there makes the distinction between "parts" and the actual "running" program even clearer.

With these things taken into consideration, your code could look like this:

open System
open System.IO

// It's a tiny bit more maintainable to keep this list separate, and makes it more readable where it is used
let separators = [|' ';'_';';';':';'(';')';'\\';'/';'>';'<';'{';'}';'0';'1';'2';'3';'4';'5';'6';'7';'8';'9';'.'|]
let fileNamePattern = "*Tests.cs"

// Function with the directory and pattern as a parameter
let getFileNames fileNamePattern directory =
    Directory.EnumerateFiles(directory, fileNamePattern, SearchOption.AllDirectories)
    |> Seq.toList

// Function with the list of filenames as a parameter
let getAllLines fileNames = fileNames |> List.collect (System.IO.File.ReadLines >> Seq.toList)

// Function with the list of lines as a parameter
let getAllWords lines =
    lines
    |> List.collect (fun (line : string) ->
        line.Split(separators, StringSplitOptions.RemoveEmptyEntries)
        |> Seq.toList)

// Reordered parameters for pipeability
let countOccurrences allWords word =
    // The = operator can be used as a function that takes two arguments.
    let count = allWords |> List.filter ((=) word) |> List.length
    word, count

// Function that takes a sequence of "anything" and returns a list of the distinct values
let distinctWords = Seq.distinct >> Seq.toList

// F# string formatting is strongly typed and compiler checked; take advantage of that, don't just use %A
let printWordCount (word, count) = printfn "%s: %i" word count

// Now that everything is "set up", we can "run" with the current data:
[<EntryPoint>]
let main args =
    // This binding isn't really necessary, but it's nice to give the value a name
    let directory = args.[0]

    // We now have a clear execution path that is very easy to follow
    let allWords =
        directory
        |> getFileNames fileNamePattern
        |> getAllLines
        |> getAllWords

    let distinct = distinctWords allWords

    distinct
    |> List.map (countOccurrences allWords)
    |> List.iter printWordCount

    0


Playing "craps" for the win No specific question here. I am beginning with Java and here is an exercise. The rules of game:

  You roll two dice. Each die has six faces, which contain one, two,
  three, four, five and six spots, respectively. After the dice have
  come to rest, the sum of the spots on the two upward faces is
  calculated. If the sum is 7 or 11 on the first throw, you win. If the
  sum is 2, 3 or 12 on the first throw (called “craps”), you lose (i.e.,
  the “house” wins). If the sum is 4, 5, 6, 8, 9 or 10 on the first
  throw, that sum becomes your “point.” To win, you must continue
  rolling the dice until you “make your point” (i.e., roll that same
  point value). You lose by rolling a 7 before making your point.

import java.io.IOException;
import java.util.Random;
import java.util.Scanner;

public class helloworld {
    private static int point;
    private static enum possibleResults {UNDEFINED, WIN, LOSE};
    private static possibleResults gameResult = possibleResults.UNDEFINED;
    private static String causeOfLose;
    private static String causeOfWin;

    public static int rollDice(int n) {
        int sum = 0;
        Random randomNumbers = new Random();
        for(int i=0; i<n; i++) {
            sum += (1 + randomNumbers.nextInt(6));
        }
        System.out.printf("Rolling dice... You got %d\n", sum);
        return sum;
    }

    public static void firstRoll() throws IOException {
        waitUser();
        int tmp = rollDice(2);
        if(tmp == 7 || tmp == 11) {
            gameResult = possibleResults.WIN;
            causeOfWin = String.format("you have got a lucky number in the first round: %d.", tmp);
        } else if(tmp == 2 || tmp == 3 || tmp == 12) {
            gameResult = possibleResults.LOSE;
            causeOfLose = String.format("you have got an unlucky number in the first round: %d.", tmp);
        } else {
            point = tmp;
            System.out.printf("Your point is %d, you need to make your point to win.%n", point);
        }
        declareResult();
    }

    public static void declareResult() {
        if(gameResult == possibleResults.LOSE) {
            System.out.printf("You have lost, because %s%n", causeOfLose);
        } else if(gameResult == possibleResults.WIN) {
            System.out.printf("You have won, because %s%n", causeOfWin);
        } else {
            System.out.println("Game continues...");
        }
    }

    public static void moreRolls() {
        Scanner inputScan = new Scanner(System.in);
        while(gameResult == possibleResults.UNDEFINED) {
            waitUser();
            int tmp = rollDice(2);
            if(tmp == point) {
                gameResult = possibleResults.WIN;
                causeOfWin = "you have made your point!";
            } else if(tmp == 7) {
                gameResult = possibleResults.LOSE;
                causeOfLose = "you have hit 7 before making your point.";
            } else {
                ;
            }
            declareResult();
        }
    }

    private static void waitUser() {
        Scanner inputScan = new Scanner(System.in);
        System.out.print("Press enter to start rolling: ");
        String input = inputScan.nextLine();
    }

    public static void main(String[] args) throws IOException {
        firstRoll();
        moreRolls();
    }

}

Any corrections and/or suggestions are welcome.

A code currently in main should be wrapped in its own method (game perhaps).
Business logic shall be performed as high in the call tree as possible. Regarding your code this principle means that neither firstRoll nor moreRolls should call declareResults. The same (questionably) applies to the while loop in moreRolls.

Consider a game skeleton along the lines of

result = firstRoll();
while (result == UNDEFINED)
    result = moreRoll();
declareResults();

moreRolls creates a Scanner and never uses it.
causeOfWin and causeOfLose are mutually dependent. You don't need two distinct variables: cause would be enough.


Take all the bits in a number and shift them to the left end I wrote this program which takes all the bits in a number and shifts them to the left end (for example: 01010110 --> 11110000). It's and exercise from a book. It works, but it seems to be very wasteful to use integers (I only need 8 bits).

Can you tell me if the code is proper or not? Is the algorithm good?

#include <iostream>

void print_bit(const int bit)
{
    for(int i = 0x80; i != 0; i >>= 1)
    {
        if((bit & i) != 0)
        std::cout << "1";
        else std::cout << "0";
    }
    std::cout << std::endl;
}

int set_bits(const int bit)
{
    int lefted = 0xFF;
    for(int i = 0x80; i != 0; i >>= 1)
    {
        if((bit & i) == 0)
            lefted <<= 1;
    }
    return lefted;
}

int main()
{
    int bit = 11;
    print_bit(bit);
    print_bit(set_bits(bit));
    return 0;
}

It appears the correct results are given, which is good.  I do see some miscellaneous things, though.

The naming and execution in main() seem confusing.  Try something like this:

int main()
{
    // the name "bit" is misleading; you're using an integer
    // name it something more accurate (this is just an idea)
    int original = 11;

    // why not return the final (shifted) integer?
    // sticking set_bit() into print_bit() is hard to read
    int final = set_bits(original);

    // print both integers
    print_bit(original);
    print_bit(final);
}

This:

if((bit & i) != 0)
std::cout << "1";
else std::cout << "0";

could become a single-line ternary statement:

// read as: (statement is true?) ? (if so, do this) : (if not, do this)
std::cout << ((bit & i) != 0) ? "1" : "0";

This:

if((bit & i) == 0)
    lefted <<= 1;

could have curly braces (to allow any additional code for the body):

if ((bit & i) == 0)
{
    lefted <<= 1;
}

I'd probably make your "magic numbers" (such as 0x80) constants.  This could help provide more context for your code, especially when there's no relevant documentation.
std::cout << std::endl; doesn't need to be in print_bit().  The display function should only print the values; that's what the user expects.  If you want a newline with the displaying, put it around the calling code.  That way, the displaying will be easier to adjust.


Better way than setInterval to wait for an image load? This is really a simple affair, I just wonder if there is a better way to do it:

let el = document.querySelectorAll('#comic img');
let loadPromise = new Promise(function(resolve, reject) {
    let ticker = setInterval(() => {
        if (el[0].complete) {
            clearInterval(ticker);
            resolve();
        }
    }, 100);
    setTimeout(() => {
        clearInterval(ticker);
        reject('Timeout');
    }, 5000)
});

loadPromise.then(() => {
    console.log(el[0].naturalWidth);
    let container = document.querySelectorAll('.main-wrapper > div > section > section.column.column-1.grid-left-4.grid-width-16');
    container[0].style.width = (el[0].naturalWidth + 420) + 'px';
});

I must wait for the image to load, so I know its size so that I can adjust an element on the page.

But setInterval(aka polling the element) seems so... medieval. Is there a nicer way in modern JS?

"Must wait for the image to load" ... "Is there a nicer way?"

Yes,
At first, instead of accessing the image by index el[0] - extract the image at once and give the variable meaningful name:

let img = document.querySelectorAll('#comic img')[0];

Next, you need to embed "load" event listener into Promise executor function to ensure the image has loaded:

let loadPromise = new Promise(function(resolve, reject) {
    img.addEventListener('load', function() {
       resolve();
    });
    setTimeout(() => {
        if (!img.complete) reject('Timeout');
    }, 5000)
});
loadPromise.then(() => {
    console.log(img.naturalWidth);
    let container = document.querySelectorAll('.main-wrapper > div > section > section.column.column-1.grid-left-4.grid-width-16');
    container[0].style.width = (img.naturalWidth + 420) + 'px';
});


Find integer pairs in an array which adds to a given sum I came up with the following solution.

Is there a better way to solve this problem than the proposed?
Also I used LinkedList to convey the result back to the API caller. Is there a better data structure to use?
I tried using a 2 dimensional array to store the pairs, but int creation requires a size, which we are not aware of until we evaluate the array. So I had to use a LinkedList where the size can be dynamic.

public class FindPairsInAnArrayAddingToASum {

    private static class ResultPair{
        ResultPair prev;
        ResultPair next;
        int a;
        int b;
    }

    /**
     * @param args
     */
    public static void main(String[] args) {
        int[] items = new int[]{1,2,3,4,5,5,6,7};
        System.out.println("Pairs: ");
        ResultPair currentNode = getPairsAddingToSum(items, 10);
        while(currentNode != null){
            System.out.println("--> "+currentNode.a+", "+currentNode.b);
            currentNode = currentNode.next;
        }
    }

    public static ResultPair getPairsAddingToSum(int[] items, int prescribedSum){
        ResultPair headNode = null;
        ResultPair currentNode = null;

        //Sort the array
        Arrays.sort(items);

        //Introduce left and right pointers
        int left = 0;
        int right = items.length-1;

        //Navigate from both directions
        while(left < right){
            if(items[left] + items[right] == prescribedSum){

                currentNode = addThePair(items[left], items[right], currentNode);

                //Save the head - Only the first time a node is created
                if(headNode == null)
                    headNode = currentNode;

                //Increment left and decrement right
                left++;
                right--;

            }else if(items[left] + items[right] < prescribedSum){
                left++;
            }else{
                right--;
            }
        }

        return headNode;
    }

    public static ResultPair addThePair(int a, int b, ResultPair currentNode){
        ResultPair newNode = new ResultPair();
        newNode.a = a;
        newNode.b = b;
        if(currentNode != null){
            currentNode.next = newNode;
            newNode.prev = currentNode;
        }
        return newNode;
    }

}

I like your approach with the two-pointer-array. As for the performance: The other answers here use the Big O notation as a basis for comparing the speed of different algorithms, hinting that using a HashMap or a HashSet might be faster than your approach. But this is misleading, since the Big O notation says nothing about the speed of different algorithms compared to each other, but only about how much slower an algorithm will be when \$n\$ increases. For example, you might have an \$O(n)\$ algorithm that takes \$n\$ hours to complete, and an \$O(n^{2})\$ algorithm that takes \$n^{2}\$ milliseconds to complete, and the second algorithm will be faster for all \$n < 3600000\$. Of course, this is an extreme example, I just wanted to make a principle clear.

As for this case, I tried to compare your algorithm with the algorithm suggested by zenwraight. Here is my implementation of that algorithm:

public class FindPairsInAnArrayAddingToASum {
    public static class IntegerPair {
        public final int number1;
        public final int number2;

        public IntegerPair(int number1, int number2) {
            this.number1 = number1;
            this.number2 = number2;
        }
    }

    public static List<IntegerPair> findNumberPairs(int[] items, int prescribedSum) {
        Map<Integer, Integer> frequencies = new HashMap<>();
        for (int item : items) {
            Integer previousFrequency = frequencies.get(item);
            frequencies.put(
                    item,
                    previousFrequency != null
                    ? previousFrequency + 1
                    : 1);
        }

        List<IntegerPair> result = new LinkedList<>();
        for (int item : frequencies.keySet()) {
            int complement = prescribedSum - item;
            Integer complementFrequency = frequencies.get(complement);
            if (complementFrequency != null
                    && (item != complement || complementFrequency > 1)) {
                result.add(new IntegerPair(item, complement));
            }
        }

        return result;
    }
}

First, I benchmarked 10 test cases where every array contains 10,000 random integers, with a different random prescribed sum for every array. Here are the results:

Double pointer array: 0.022 seconds
Hash map: 0.066 seconds

Double pointer array: 0.012 seconds
Hash map: 0.036 seconds

Double pointer array: 0.017 seconds
Hash map: 0.021 seconds

Double pointer array: 0.003 seconds
Hash map: 0.071 seconds

Double pointer array: 0.006 seconds
Hash map: 0.013 seconds

Double pointer array: 0.006 seconds
Hash map: 0.009 seconds

Double pointer array: 0.008 seconds
Hash map: 0.01 seconds

Double pointer array: 0.003 seconds
Hash map: 0.009 seconds

Double pointer array: 0.003 seconds
Hash map: 0.007 seconds

Double pointer array: 0.002 seconds
Hash map: 0.008 seconds

So your code performed slightly better in all test cases listed here (although there also were some 10,000-size array test cases in other benchmark runs where the HashMap performed slightly better). Next, 10 test cases, same as above, but now the arrays contain 100,000 integers:

Double pointer array: 0.064 seconds
Hash map: 0.346 seconds

Double pointer array: 0.068 seconds
Hash map: 0.381 seconds

Double pointer array: 0.03 seconds
Hash map: 0.068 seconds

Double pointer array: 0.032 seconds
Hash map: 0.12 seconds

Double pointer array: 0.032 seconds
Hash map: 0.17 seconds

Double pointer array: 0.037 seconds
Hash map: 0.073 seconds

Double pointer array: 0.014 seconds
Hash map: 0.12 seconds

Double pointer array: 0.013 seconds
Hash map: 0.054 seconds

Double pointer array: 0.016 seconds
Hash map: 0.059 seconds

Double pointer array: 0.014 seconds
Hash map: 0.066 seconds

Your code still wins. The differences are even more significant in some cases than when the arrays only contained 10,000 integers. Next, increase the array size to 1,000,000 integers:

Double pointer array: 0.495 seconds
Hash map: 2.356 seconds

Double pointer array: 0.336 seconds
Hash map: 3.916 seconds

Double pointer array: 0.279 seconds
Hash map: 0.651 seconds

Double pointer array: 0.164 seconds
Hash map: 0.713 seconds

Double pointer array: 0.162 seconds
Hash map: 0.885 seconds

Double pointer array: 0.31 seconds
Hash map: 0.868 seconds

Double pointer array: 0.164 seconds
Hash map: 0.795 seconds

Double pointer array: 0.165 seconds
Hash map: 0.919 seconds

Double pointer array: 0.323 seconds
Hash map: 0.71 seconds

Double pointer array: 0.164 seconds
Hash map: 0.697 seconds

Your double pointer array beats the hash map again. Finally, increase the array size to 5,000,000 (the program ran out of memory when I tried to run it with 10,000,000-size arrays, probably because I generated all 10 test cases in advance instead of only generating one for each benchmark).

Double pointer array: 1.625 seconds
Hash map: 14.116 seconds

Double pointer array: 0.927 seconds
Hash map: 12.554 seconds

Double pointer array: 0.914 seconds
Hash map: 11.51 seconds

Double pointer array: 0.924 seconds
Hash map: 10.654 seconds

Double pointer array: 1.013 seconds
Hash map: 15.335 seconds

Double pointer array: 1.039 seconds
Hash map: 10.512 seconds

Double pointer array: 0.9 seconds
Hash map: 11.928 seconds

Double pointer array: 0.954 seconds
Hash map: 10.16 seconds

Double pointer array: 0.916 seconds
Hash map: 12.326 seconds

Double pointer array: 0.899 seconds
Hash map: 9.881 seconds

I don't know why the HashMap algorithm takes so long here. It could have to do with the need to increase the capacity of the HashMap multiple times, but experimenting with different values for the initial capacity didn't make much of a difference. But anyway, I think this makes it clear that you shouldn't rely on the Big O notation when comparing the speed of different algorithms to each other.

However, apart from the performance, your public method getPairsAddingToSum(int[], int) returns a ResultPair, which is a private class, so whoever calls your method won't be able to do anything with the returned value. But regardless of that, I don't understand why, in your code, you invent something that already exists, namely a linked list. Why don't you just use java.util.LinkedList?

In any case, you also asked whether there would be a better data structure than a linked list to return the found pairs. I think there is, namely a Set. After all, the array might contain duplicates, so it might also contain duplicate valid pairs. Of course, in order to take advantage of a Set, you would have to implement hashCode() and equals(Object) in your ResultPair class (which would then only contain the two integers and no nodes). Seeing as, in your algorithm, the first number in a pair will always be less than or equal to the second number, it would suffice for ResultPair.equals(Object) to compare a and b from both ResultPairs to each other without sorting them first, thereby considering to ResultPairs with the same numbers but in a different order unequal, but this depends on how you want to use the class ResultPair further (in case you do want to use it outside the context of this algorithm). Note that, if you use a Set, the algorithm will naturally take longer than if you use a List, because removing duplicates is an additional task that was not carried out in your original code.

Edit:

Actually, there's a much simpler and more efficient approach for avoiding duplicate number pairs than using a Set: When adjusting the array counters for the next while loop, you could simply in-/decrement them until the integer they point to is different from the integer they previously pointed to, instead of in-/decrementing them only once.


React - am i overusing props or is it exactly how they are suppoused to be used? After learning some basic concepts of ReactJs I tried to create a form. Now I'm not sure if I'm using props the way they are intented so here is sample of my relevant code:

<Input type={"text"} placeholder={"Username"} id={"registerUsername"} class={"input"} />
<Input type={"password"} placeholder={"Password"} id={"registerPassword"} class={"input"} />

Input class:

export class Input extends React.Component{
 render(){
  return (
   <input type={this.props.type} placeholder={this.props.placeholder} id={this.props.id} className={this.props.class} />
  )
 }
}

When passing many props in one component for whatever reason it feels like I'm overusing them, but maybe that's just me?

Use Props !

Using props to highly customize your component is good practice, but too many props isn't always good ! You need to ask yourself:

Am I trying to overly handle every situation possible ?
Is it ok if a second component handles different cases ?
Are these props absolutely required for my component to work as intended ?

If 1. and/or 2. is a yes and/or 3. is a no then you should probably rethink your component logic.

For your example, you're not overly using props.

Don't assume your props exist !

<input type={this.props.type} .../>

Get the props and set default values for them if they are undefined. Use destructuring for this:

render(){
  const {
      type        = "text",
      placeholder = "Type something",
      id          = null,
      className   = null
  } = this.props;

  return (
   <input type={type} placeholder={placeholder} id={id} className={className} />
  )
 }

Use PropTypes !

  You can use prop-types to document the intended types of properties passed to components. React (and potentially other libraries—see the checkPropTypes() reference below) will check props passed to your components against those definitions, and warn in development if they don’t match.

Using many props gets messy and it's easy to forget which props are being used and which props are required for your components.

Example:

export class Input extends React.Component{
   /** code here */
}

Input.propTypes = {
  id: PropTypes.number.isRequired, //required will throw error if id is not set
  placeholder: PropTypes.string,
  className: PropTypes.string, //optional
  type: PropTypes.string, //optional
}

When to use classes and when not to use classes

Keep in mind that all your components don't need to be a class. Only use a class if the object becomes complex.

Keep things simple like so:

export const Input = props => {

  const {
      type        = "text",
      placeholder = "Type something",
      id          = null,
      className   = null
  } = props;

  return (
   <input
     type={type}
     placeholder={placeholder}
     id={id}
     className={className} />
  )
}


Parsing a text file into a pandas DataFrame I have a .txt file that looks like this:
SHT1 E:   T1:30.45°C    H1:59.14 %RH
SHT2 S:   T2:29.93°C    H2:67.38 %RH

SHT1 E:   T1:30.49°C    H1:58.87 %RH
SHT2 S:   T2:29.94°C    H2:67.22 %RH

SHT1 E:   T1:30.53°C    H1:58.69 %RH
SHT2 S:   T2:29.95°C    H2:67.22 %RH

I want to have a DataFrame that looks like this:
      T1     H1     T2     H2
0  30.45  59.14  29.93  67.38
1  30.49  58.87  29.94  67.22
2  30.53  58.69  29.95  67.22

I parse this by:

Reading up the text file line by line
Parsing the lines e.g. matching only the parts with T1, T2, H1, and H2, splitting by :, and removing °C and %RH
The above produces a list of lists each having two items
I flatten the list of lists
Just to chop it up into a list of four-item lists
Dump that to a df
Write to an Excel file

Here's the code:
import itertools

import pandas as pd

def read_lines(file_object) -> list:
    return [
        parse_line(line) for line in file_object.readlines() if line.strip()
    ]

def parse_line(line: str) -> list:
    return [
        i.split(":")[-1].replace("°C", "").replace("%RH", "")
        for i in line.strip().split()
        if i.startswith(("T1", "T2", "H1", "H2"))
    ]

def flatten(parsed_lines: list) -> list:
    return list(itertools.chain.from_iterable(parsed_lines))

def cut_into_pieces(flattened_lines: list, piece_size: int = 4) -> list:
    return [
        flattened_lines[i:i + piece_size] for i
        in range(0, len(flattened_lines), piece_size)
    ]

with open("your_text_data.txt") as data:
    df = pd.DataFrame(
        cut_into_pieces(flatten(read_lines(data))),
        columns=["T1", "H1", "T2", "H2"],
    )
    print(df)
    df.to_excel("your_table.xlsx", index=False)

This works and I get what I want but I feel like points 3, 4, and 5 are a bit of redundant work, especially creating a list of list just to flatten it and then chop it up again.

Question:
How could I simplify the whole parsing process? Or maybe most of the heavy-lifting can be done with pandas alone?
Also, any other feedback is more than welcomed.

Disclaimer: I know this is a very liberal interpretation of a code review since it suggests an entirely different approach. I still thought it might provide a useful perspective when thinking about such problems in the future and reducing coding effort.
I would suggest the following approach using regex to extract all the numbers that match the format "12.34".
import re
import pandas as pd

with open("your_text_data.txt") as data_file:
    data_list = re.findall(r"\d\d\.\d\d", data_file.read())

result = [data_list[i:i + 4] for i in range(0, len(data_list), 4)]

df = pd.DataFrame(result, columns=["T1", "H1", "T2", "H2"])
print(df)
df.to_excel("your_table.xlsx", index=False)

This will of course only work for the current data format you provided. The code will need to be adjusted if the format of your data changes. For example: If relevant numbers may contain a varying number of digits, you might use the regex "\d+\.\d+" to match all numbers that contain at least one digit on either side of the decimal point.
Also please note the use of the context manager with open(...) as x:. Only code that accesses the object needs to and should be part of the managed context.


Scala Play Controller - Refactoring out Duplication for Restful Web Services I'm new to scala and I'm noticing I have some repeating code in my controllers.

I'm wondering if anyone can give some hints for traits and parameterization for eliminating duplicate code for basic crud operations on the web services - I'm still a little confused about how to use scala 'generics'/parameters.

Don't mind the json - I've fixed this horrible scrappy code now using the objects to render themselves as json.

Code piece

package controllers

import play.api.mvc._
import securesocial.core._
import play.Logger
import models.{ResponseMessage, GraphedUser}
import com.tinkerpop.blueprints.Vertex
import service.{BaseFramedEntity, SecureSocialUserService, FramedGraphService, GraphDbService}
import play.api.libs.json._
import scala.util.parsing.json
import json.JSONArray
import play.api.libs.concurrent.{Akka, Promise}
import play.api.Play.current
import securesocial.core.UserId
import Predef._
import play.mvc.With

/**
 * Controller for user api
 */
object UsersController extends BaseController with securesocial.core.SecureSocial {

  val collectionName = "users"

  def index = Action {
    implicit request =>
    //get all users

      val pageOption = request.queryString.get("page")

      pageOption match {
        case Some(page) => {
          val resultsPerPageOption = request.queryString.get("numResults")
          resultsPerPageOption match {
            case Some(size) => Ok(renderUserJson(getUserList(page.head.toInt, size.head.toInt))) //Both page and size exists
            case None => Ok(renderUserJson(getUserList(page.head.toInt, defaultPageSize))) //only page exists
          }
        }
        case None => Ok(renderUserJson(getUserList(defaultPage, defaultPageSize))) //page doesn't exist. Ignore size.
      }
  }

  def view(id: String) = Action {
    implicit request =>

      val userOption = FramedGraphService.getFramedVertexByProperty("userId", id, classOf[GraphedUser])

      userOption match {
        case Some(user) => {
          Ok(user.toString)
        }
        case None => InternalServerError("Could not find user")
      }
  }

  def follow(id: String) = SecuredAction(true) {
    implicit request =>

    //Must validate user...
      val currentUserIdentityOption = SecureSocial.currentUser

      currentUserIdentityOption match {
        case Some(currentUser) => {

          val userToFollowOption = FramedGraphService.getFramedVertexByProperty("userId", id, classOf[GraphedUser])

          userToFollowOption match {
            case Some(userToFollow) => {
              GraphDbService.addEdge(null, currentUser.id.id, id, "follows")
              val message = ResponseMessage("ok", "following user: " + id.toString).toJsonString
              Ok(message)
            }
            case None => Ok(ResponseMessage("error", "cannot find user: " + id.toString).toJsonString)
          }
        }
        case None => InternalServerError(ResponseMessage("error", "unknown error").toJsonString)
      }
  }

  private def getUserList(page: Int, size: Int): Iterable[GraphedUser] = {
    val fullUsersList = FramedGraphService.getFramedVerticesByProperty(BaseFramedEntity.TYPE_KEY, GraphedUser.USER_TYPE, classOf[GraphedUser])
    fullUsersList.slice((page - 1) * size, (page - 1) * size + size)
  }

  private def renderUserJson(users: Iterable[GraphedUser]): String = {

    val listToConvert = for (user <- users) yield
      Json.toJson(Map(
        "userId" -> Json.toJson(user.getUserId),
        "graphId" -> Json.toJson(user.asVertex().getId.toString),
        "firstName" -> Json.toJson(user.getFirstName),
        "lastName" -> Json.toJson(user.getLastName),
        "fullName" -> Json.toJson(user.getFullName),
        "providerId" -> Json.toJson(user.getProviderId),
        "firstName" -> Json.toJson(user.getAvatarUrl)))

    val concatenated = listToConvert.foldLeft("")(_ + _)
    surroundJsonWithObject(concatenated, collectionName)
  }
}

for all the option matchers you could use getOrElse
And remember: Option is also a list with all given features

here an example

val resultsPerPageOption = request.queryString.get("numResults")
val size= resultsPerPageOption.map(_.head.toInt).getOrElse(defaultPageSize)
Ok(renderUserJson(getUserList(page.head.toInt, size)))


AES File Encryption / Decription VB.NET I've been looking for a while for the right code to encrypt/decrypt files with AES in vb.net. I created the code below. Is this the right way doing AES encryption/decryption nowadays?

Public Function EncryptFile(ByVal plainFilePath As String, ByVal encryptedFilePath As String, ByVal EncryptionKey As String, ByVal SaltValue As String) As String
    Dim passwordIterations As Integer = 1000
    Dim initVector As String = "Bla3212EWMND12es"
    Dim initVectorBytes As Byte() = Encoding.ASCII.GetBytes(initVector)
    Dim saltValueBytes As Byte() = Encoding.ASCII.GetBytes(SaltValue)
    Dim k1 As New Rfc2898DeriveBytes(EncryptionKey, saltValueBytes, passwordIterations)
    Dim symmetricKey As New RijndaelManaged()
    symmetricKey.Mode = CipherMode.CBC
    Dim encryptor As ICryptoTransform = symmetricKey.CreateEncryptor(k1.GetBytes(16), initVectorBytes)

    Using plain As FileStream = File.Open(plainFilePath, FileMode.Open, FileAccess.Read, FileShare.Read)
        Using encrypted As FileStream = File.Open(encryptedFilePath, FileMode.Create, FileAccess.Write, FileShare.None)
            Using cs As CryptoStream = New CryptoStream(encrypted, encryptor, CryptoStreamMode.Write)
                plain.CopyTo(cs)
            End Using
        End Using
    End Using
End Function

Public Function DecryptFile(ByVal plainFilePath As String, ByVal encryptedFilePath As String, ByVal DecryptionKey As String, ByVal SaltValue As String) As String
    Dim passwordIterations As Integer = 1000
    Dim initVector As String = "Bla3212EWMND12es"
    Dim initVectorBytes As Byte() = Encoding.ASCII.GetBytes(initVector)
    Dim saltValueBytes As Byte() = Encoding.ASCII.GetBytes(SaltValue)
    Dim k1 As New Rfc2898DeriveBytes(DecryptionKey, saltValueBytes, passwordIterations)
    Dim symmetricKey As New RijndaelManaged()
    symmetricKey.Mode = CipherMode.CBC
    Dim decryptor As ICryptoTransform = symmetricKey.CreateDecryptor(k1.GetBytes(16), initVectorBytes)

    Using plain As FileStream = File.Open(plainFilePath, FileMode.Create, FileAccess.Write, FileShare.None)
        Using encrypted As FileStream = File.Open(encryptedFilePath, FileMode.Open, FileAccess.Read, FileShare.Read)
            Using cs As CryptoStream = New CryptoStream(plain, decryptor, CryptoStreamMode.Write)
                encrypted.CopyTo(cs)
            End Using
        End Using
    End Using
End Function

Security is hard. The recommended approach to implementing anything crypto related is let someone else do it! When I first thought, before reading the code, that you were actually trying to implement AES from scratch by hand in VB, I was just going to say don't. You aren't doing that, but there are still a lot of fiddly details to worry about for a secure system.

Dim initVector As String = "Bla3212EWMND12es"

The purpose of an initialisation vector is to randomise the subsequent stream. The way that AES under CBC (or one of its successors) works is that you encrypt the first block with the key, the second block with the key and the first block, the the third block with the key and the second block, and so on. This means that even if two blocks contain the same data, they won't be the same once they're encrypted.

However, this does mean that if two messages are the same, they'll encrypt to the same thing. Even if two messages are different but the first 100 blocks are the same, the first hundred blocks will encyrpt to the same thing. This might not sound like much, but it gives valuable information to an attacker.

Solution: we use initialisation vectors. If you start your message off with one block of random junk, even two identical messages will (assuming an ideal cipher) have no more in common than with the equivalent amount of random noise. For this to work, initialisation vectors must be different each time. (The initialisation vector is not secret: you can happily stick it onto the front of your encrypted file in plaintext so the decrypt side knows how to start off. But generate it with your favourite cryptographically secure random number generator.)

symmetricKey.Mode = CipherMode.CBC

I've briefly described the idea behind CBC above. Of the available ciphermodes, it is not a bad one for encrypting static files although there have been some quite famous attacks on it when used to encrypt network traffic. (Basically, you should use anything but ECB!)

ByVal EncryptionKey As String

Darting right back to the top, be careful with security data. From the context I'm infering that this is a password of some description, presumably user-supplied. Whereever it comes from, crypto libraries do all sorts of magic to make sure that secret data (especially passwords and keys) get wiped out of RAM once used and don't get accidentally written to the page file or something. I am not familar enough with VB.net to say how you'd do that, but you should consider your chain of custody here.

Dim symmetricKey As New RijndaelManaged()

Per MSDN:

  The Rijndael algorithm is the predecessor of Aes. You should use the Aes class instead of RijndaelManaged. For more information, see the entry The Differences Between Rijndael and AES in the .NET Security blog.

plain.CopyTo(cs)

It's worth thinking about what you want to happen to your plaintext file. I very much would not expect anything about it to change in an EncryptFile function, and very often you do just want it to just stay there and do nothing. However, I'll at least mention it in case you don't want a copy of the original: in that case you want to securely delete it. There's nothing quite so satisfying for an attacker as ignoring all your fancy encryption and reading the unencrypted original straight from disk!


Handling `IO Bool` and `IO (Either a b)` inside `IO (Maybe b)` {-# LANGUAGE LambdaCase          #-}
{-# LANGUAGE ScopedTypeVariables #-}

data E a
  = E1
  | E2
  | E3 a

data L
data R

f :: IO (Maybe (E L))
f =
  (undefined :: IO Bool) >>= \case
    False -> return (Just E1)
    True  ->
      (undefined :: IO Bool) >>= \case
        False -> return (Just E2)
        True  ->
          (undefined :: IO (Either L R)) >>= \case
            Left e  -> return (Just (E3 e))
            Right v -> (undefined :: R -> IO ()) v >> return Nothing

How to get rid of these three nested cases?

Since you need to abort your computation early and return a value different from the final output, the ExceptT transformer would be useful.

import Control.Monad (unless)
import Control.Monad.IO.Class
import Control.Monad.Trans.Except

data E a
  = E1
  | E2
  | E3 a

data L
data R

f :: ExceptT (E L) IO ()
f = do
  liftIO (undefined :: IO Bool) >>= flip unless (throwE E1)
  liftIO (undefined :: IO Bool) >>= flip unless (throwE E2)
  liftIO (undefined :: IO (Either L R)) >>=
    either (throwE . E3) (liftIO . (undefined :: R -> IO ()))

If we want, we can then convert ExceptT (E L) IO () to IO (Maybe (E L)):

f' :: IO (Maybe (E L))
f' = either Just (const Nothing) <$> runExceptT f

If could be further polished by using helper functions from Control.Conditional or a similar library, and by extending the functions, for which undefined fills in, to work within any MonadIO (if that's possible).


Fetch text values from JSON and Display pop onclick text I have below code which will fetch the text from json & onclick text , display pop up box in page.... Now i need to give options to change font-family, font color....

But Adding the code in the place of Content is going to be extremely difficult....

Is there any way so that i can improve code structure or write same code in another way ?

JSON :

{
          "font" : "Arian",
          "x" : 201,
          "y" : 461,
          "src" : "180ef82d1.otf",
          "width" : 679,
          "type" : "text",
          "text" : "Good Food",
          "name" : "edit_good_1"
}

script

const lightId = 'light' + layer.name
        const lightIdString = '#' + lightId
        $('.container').append(
            '<a id ="' + layer.name + '" onclick="openPopUp(' + lightId + ')"' +
            '
' + layer.text + '
' +
            '
 content Close

'
        );
        document.getElementById(lightId).style.left = layer.x + document.getElementById(layer.name).offsetWidth + 'px'

codepen : https://codepen.io/kidsdial/pen/OGbGwN

fiddle : https://jsfiddle.net/kidsdial1/z6eyq4j3/

Your code is very prone to errors, since you write all your html tags and content as a single string. Containing html and javascript in that way is extremely unsafe.

I would suggest creating your elements with document.createElement() and then using the JSON to set all the attributes. In the function you could just pass the id, and then find the correct json data for that id.

Concept code

function buildPopup(id) {
  let layer = findJsonData(id)

  let popup = document.createElement("div")
  popup.classList.add("txtContainer")
  popup.id = layer.name
  popup.style.left = layer.x + "px"
  popup.style.top = layer.y + "px"
  popup.style.fontFamily = layer.font
  popup.innerHTML = layer.text

  // adding a close button
  let btn = document.createElement("button")
  popup.appendChild(btn)
  btn.addEventListener("click", ()=>{
      closePopup(layer.name)
  })

  document.body.appendChild(popup)
}

Example for calling this function

 Open popup

Example for getting the json by id

 // this function will find the right json data for this id
 function findJsonData(id){
     return {
      "font" : "Arian",
      "x" : 201,
      "y" : 461,
      "src" : "180ef82d1.otf",
      "width" : 679,
      "type" : "text",
      "text" : "Good Food",
      "name" : "edit_good_1"
    }
 }

I realise this is a bit conceptual but hopefully it will point you in the right direction!

JSFiddle


Copy-and-Move Concept Using Smart Pointer I tried to implement a simple container class using the copy-and-move idiom. I am learning the changes to C++ as of C++11 and later.

Is the code exception safe?
I chose to use a smart pointer. Is std::unique_ptr a safe choice? I read that it should not be copied, but since I am doing the copying of the content explicitly, and since it provides a swap function, I could not see any objection. Is there a preferred practice, considering the array content type might be any type including an object type?
When I let the compiler do the copy for the copy assignment operator (like ...::operator=(MyContainer other)) I got an ambiguity with the move assignment operator. So I chose to put the copy operation into the function (which I prefer visually, it might look naive and oldschool, but as an aging brain I find it more explicit). Also here: Is there another, preferred practice? And to overcome my personal preference: What is the real difference of letting the compiler do the copy? Both are taking place on the stack, aren't they?

Header part

class MyContainer
{
private:
    std::size_t mSize;
    std::unique_ptr<int[]> mArray;

    static void _swap(MyContainer& first, MyContainer& second);
    void _reset(void);

public:
    MyContainer(std::size_t size); // default constructor
    MyContainer(const MyContainer& other); // copy constructor
    MyContainer(MyContainer&& other) noexcept; // move constructor
    MyContainer& operator=(const MyContainer& other); // copy assignment operator
    MyContainer& operator=(MyContainer&& other) noexcept; // move assignment operator
    void dump(void);
    /* ... */
    ~MyContainer(void);
};

Source part

void MyContainer::_swap(MyContainer& first, MyContainer& second) // static
{
    std::swap(first.mSize, second.mSize);
    first.mArray.swap(second.mArray);
}

void MyContainer::_reset(void)
{
    mSize = 0;
    mArray.reset();
}

MyContainer::MyContainer(std::size_t size = 0) // default constructor
    : mSize(size),
    mArray(mSize ? new int[mSize] : nullptr)
{
    // for testing purpose only
    for (int i = 0; i < mSize; i++)
        mArray.get()[i] = i;
}

MyContainer::MyContainer(const MyContainer& other) // copy constructor
    : mSize(other.mSize),
    mArray(mSize ? new int[mSize] : nullptr)
{
    std::copy(other.mArray.get(), other.mArray.get() + mSize, mArray.get());
}

MyContainer::MyContainer(MyContainer&& other) noexcept // move constructor
    : MyContainer() // init using default constructor (C++11)
{
    _swap(*this, other);
}

MyContainer& MyContainer::operator=(const MyContainer& other) // copy assignment
{
    MyContainer tmp(other);
    _swap(*this, tmp);
    return *this;
}

MyContainer& MyContainer::operator=(MyContainer&& other) noexcept // move assignment
{
    if (this != &other)
    {
        _reset();
        _swap(*this, other);
    }
    return *this;
}

void MyContainer::dump(void)
{
    std::cout << "Size: " << mSize << std::endl;
    for (int i = 0; i < mSize; i++)
        std::cout << mArray.get()[i] << std::endl;
}

MyContainer::~MyContainer(void)
{
}

unique_ptr is fine here. It's used where you would have used a plain pointer before.

But don't use naked new — use make_unique.  Oh, and I think a unique_ptr of array types was not in C++11 but needs C++14.

Don’t use (void) for () in parameter lists!  That was never proper, even in c++98. (it was supported to allow compatibility with C headers, but in Bjarne’s own words it’s “an abomination”.

Don’t write an empty destructor.  Use =default to emphasize that it exists, or just leave it off completely.

Why have a private _swap static function instead of a non-member overload of swap that anyone can use?

Another important new feature you have not embraced yet is the range for loop.  Use that instead of the lagacy for loops you have that go over the whole array.


Extracting name and value from the enum I wrote the below enum from which I need to extract the name and its value:

public enum UserEnum {
    TreeUser("/tree"), ParentUser("/parent");

    private String value;

    UserEnum(String value) {
        this.value = value;
    }

    public String value() {
        return value;
    }
}

This is the way I am using the above UserEnum in my code base:

// extract TreeUser
String nameOfTree = UserEnum.TreeUser.name();
String valueOfTree = UserEnum.TreeUser.value();

// extract ParentUser
String nameOfParent = UserEnum.ParentUser.name();
String valueOfParent = UserEnum.ParentUser.value();

// and I am using UserEnum this way as well to make name1=value1,name2=value2
String mapping = UserEnum.TreeUser.name() + "="
                + UserEnum.TreeUser.value() + "," + UserEnum.ParentUser.name() + "="
                + UserEnum.ParentUser.value();

I am opting for a review to see whether I can simplify anything in my enum.

Not sure about the usage of nameOfTree , nameOfParent and same for the value. But one thing to simplify your mapping and provide a representation for your enum is:

public String toString(){
    return  this.name() + "=" + this.value()
}

// and mapping cna be written as.
String mapping = UserEnum.TreeUser.toString() +  "," + UserEnum.ParentUser.toString();
String mapping = UserEnum.TreeUser +  "," + UserEnum.ParentUser; // toString() is called implictly.


Installing GNOME extension via CLI Intending to reuse the installation process of a GNOME extension, I wrote the following script:

#!/bin/bash
# It installs a target gnome shell extension defined via url

STORE_URL="https://extensions.gnome.org/extension-data"
EXTENSIONS_PATH="$HOME/.local/share/gnome-shell/extensions/"
ZIP="gnome-extension.zip"

cd ~/Downloads
wget $STORE_URL/$1 -O ./$ZIP
UUID=$(unzip -c $ZIP metadata.json | grep uuid | cut -d \" -f4)

if [[ ! -d $EXTENSIONS_PATH/$UUID ]]; then
    mkdir $EXTENSIONS_PATH/$UUID
    unzip -q ./$ZIP -d $EXTENSIONS_PATH/$UUID
    gnome-shell-extension-tool -e $UUID
fi

rm $ZIP
gnome-shell --replace &

  It expects to receive as argument the target extension zip name, one available from official GNOME extensions index. The zip file, after downloaded, will be unzipped in its UUID directory inside GNOME extensions' location and then enable.

Is there something else could be done to improve this installation process?
Any of the executed commands could be improved or code get better readability?

First off, install Shellcheck and routinely run it on your scripts, or configure your editor to run it automatically. Now, Shellcheck is pretty rigorous and you would be forgiven for not solving all its warnings, but at the very least be aware of them.

#!/bin/bash

On some systems there’s a more up to date bash installed in a different location. In general you’d want to use
#!/usr/bin/env bash

Though in the case of Bash on Linux I think yours is OK.1
Next, enable stricter error checking:
set -euo pipefail

From help set:

-e  Exit immediately if a command exits with a non-zero status.
-u  Treat unset variables as an error when substituting.
-o pipefail the return value of a pipeline is the status of
the last command to exit with a non-zero status,
or zero if no command exited with a non-zero status

Regarding your variables, there’s a convention to use UPPER_CASE for environment variables, and snake_case for others. Not mandatory, but a nice way to distinguish them.

wget $STORE_URL/$1 -O ./$ZIP

Even though in this specific case there are no spaces or other special characters in your variables, always double-quote them to prevent string splitting — there’s simply no reason ever not to do so, and it gets you into the habit of spotting potential errors when quotes are missing.

cd ~/Downloads

That directory might or might not exist. As we’re talking about a GNOME environment, you might want to use something like $(xdg-user-dir DOWNLOAD) to get the actual download directory. The same is usually true for the path ~/.local/share, but the GNOME shell extensions documentation makes it sound as if the path is actually hard-coded.

rm $ZIP

This will not be executed if the script failed earlier due to an error. To ensure cleanup on error or if the user aborted the script, replace this with a trap.
Resulting script:
#!/usr/bin/env bash

# Install a target gnome shell extension defined via url

set -euo pipefail

store_url="https://extensions.gnome.org/extension-data"
extensions_path="$HOME/.local/share/gnome-shell/extensions/"
zip="gnome-extension.zip"

cd "$(xdg-user-dir DOWNLOAD)"
wget "$store_url/$1" -O "$zip"
trap 'rm "$zip"' ERR INT TERM EXIT

uuid=$(unzip -c "$zip" metadata.json | grep uuid | cut -d \" -f4)

if [[ ! -d "$extensions_path/$uuid" ]]; then
    mkdir "$extensions_path/$uuid"
    unzip -q "$zip" -d "$extensions_path/$uuid"
    gnome-shell-extension-tool -e "$uuid"
fi

gnome-shell --replace &

1 On macOS, on the other hand, it’s a bad idea since the Bash version of /bin/bash is eternally stuck at 3.2 due to licensing issues, and if you want to use any more recent Bash features you need to rely on a user-installed version with a different path.


3-way multiplexer built on 2-way multiplexers in Verilog I created this project on EDA Playground that builds a variable-width 3-way multiplexer out of 2 variable-width 2-way multiplexers. It will be part of homework that I assign, so I want to follow all best practices. I followed the lowRISC Verilog Coding Style Guide.
Design Modules
module mux2 #(
  parameter Width = 4
) (
  input              control,
  input [Width-1:0]  a,
  input [Width-1:0]  b,
  output [Width-1:0] y
);

  assign y = control ? b : a;

endmodule

module mux3 #(
  parameter Width = 4
) (
  input [1:0]        control,
  input [Width-1:0]  a,
  input [Width-1:0]  b,
  input [Width-1:0]  c,
  output [Width-1:0] y
);

  wire [Width-1:0] mux_a_out;

  mux2 #(
    .Width   (Width)
  ) mux_a (
    .control (control[0]),
    .a       (a),
    .b       (b),
    .y       (mux_a_out)
  );

  mux2 #(
    .Width   (Width)
  ) mux_b (
    .control (control[1]),
    .a       (mux_a_out),
    .b       (c),
    .y       (y)
  );

endmodule

Test Module
module test;
  reg[1:0] control;
  reg[7:0] a;
  reg[7:0] b;
  reg[7:0] c;
  reg[7:0] y;

  mux3 #(
    .Width(8)
  ) mux3(
    .control (control),
    .a       (a),
    .b       (b),
    .c       (c),
    .y       (y)
  );

  initial begin
    a = "A";
    b = "B";
    c = "C";

    for (int i = 0; i < 3; i++)
      begin
        control = i;
        #1
        $display("When Control = %d, Y = %c", control, y);
      end
  end

endmodule

I don't see any problems with the code functionally, and the layout follows good practices for the most part.  That style guide seems mostly reasonable, and it is a good idea to use a set of guidelines.
In my many years of Verilog coding, I have most frequently seen parameters use all capital letters (this departs from the guide you posted):
Use WIDTH instead of Width.
The guide recommends 2-space indents in some places and 4 in others; I recommend 4-space indent everywhere because I think it is much easier to read.
I don't think  your for loop follows the guide.  I would lay it out like this:
for (int i = 0; i < 3; i++) begin
    control = i;
    #1
    $display("When Control = %d, Y = %c", control, y);
end

I would also add a space between reg and [:
  reg [1:0] control;
  reg [7:0] a;
  reg [7:0] b;
  reg [7:0] c;
  reg [7:0] y;

I usually find it helpful for debugging to also display the time (copied from my Answer to your previous Verilog Question):
$display($time, " When Control = %d, Y = %c", control, y);

The for loop exercises 3 of the 4 possible values of Control.  It is a good practice to simulate all possibilities (Control = 3).  Perhaps make it an extra-credit question to explain what happens and why.

I assume one of the goals of this assignment is to demonstrate module hierarchy using an extremely simple design.  Otherwise, your design modules normally would be replaced with a single assign statement.


OpenGL VertexArrayObject class It seems to play up sometimes and I'm not sure if this is due to the class itself or the way I'm using it. Can someone confirm that this is a 'good' implementation of a VertexArrayObject class?

//The following code is public domain

public class VertexArray : GraphicsResource
{
    protected readonly int ID;
    DrawMode _drawMode;
    VertexLayout _vertexType;

    protected BufferUsageHint bufferUsage = BufferUsageHint.StaticDraw;
    int vertexCount;
    int vertexBufferID;

    public DrawMode DrawMode
    {
        get { return _drawMode; }
        set { _drawMode = value; }
    }
    public int VertexCount
    {
        get { return vertexCount; }
    }
    public BufferUsageHint BufferUsage
    {
        get { return bufferUsage; }
        set { bufferUsage = value; }
    }

    public VertexArray()
        : base()
    {
        ID = createVAO();
        disposed = false;
    }
    public VertexArray(DrawMode drawMode)
        : base()
    {
        _drawMode = drawMode;
        ID = createVAO();
        disposed = false;
    }
    int createVAO()
    {
        int id;
        GL.GenBuffers(1, out vertexBufferID);
        GL.GenVertexArrays(1, out id);
        GL.BindVertexArray(id);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        VenGFX.currentVAO = this;
        return id;
    }
    public void Bind()
    {
        if (vertexCount == 0)
            throw new Exception("Vertex buffer has no data, and therefore can't be used");
        if (disposed)
            throw new ObjectDisposedException(ToString());
        GL.BindVertexArray(ID);
        VenGFX.currentVAO = this;
    }
    public void SetData<T>(T[] Array) where T : struct, IVertex
    {
        GL.BindVertexArray(ID);
        if (Array.Length > 0 && Array[0].Layout != _vertexType)
            setVertexType(Array[0].Layout);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(Array.Length * _vertexType.SizeInBytes), Array, bufferUsage);
        vertexCount = Array.Length;
        VenGFX.currentVAO = this;
    }
    public void SetData<T>(T[] Array, int length) where T : struct, IVertex
    {
        GL.BindVertexArray(ID);
        if (Array != null && Array.Length > 0 && Array[0].Layout != _vertexType)
            setVertexType(Array[0].Layout);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(length * _vertexType.SizeInBytes), Array, bufferUsage);
        vertexCount = length;
        VenGFX.currentVAO = this;
    }
    void setVertexType(VertexLayout layout)
    {
        _vertexType = layout;
        layout.Set();
    }
    protected override void Release()
    {
        if (VenGFX.currentVAO == this)
        {
            GL.BindVertexArray(0);
            VenGFX.currentVAO = null;
        }

        int id = ID;
        GL.DeleteBuffers(1, ref vertexBufferID);
        GL.DeleteVertexArrays(1, ref id);
    }
}

To draw with it, I write

if (mesh.VertexCount == 0)
    return;
if (currentVAO != mesh)
    mesh.Bind();

GL.DrawArrays((BeginMode)(int)mesh.DrawMode, 0, mesh.VertexCount);

I know nothing of OpenGL and what a correct implementation of a VertexArrayObject should look like, but I do have a couple of observations:

I like that the ID private field is readonly, ...but its naming breaks the naming convention for private fields, which should be either id or _id - looking at the other private fields _id would be more consistent.
I don't see why the other fields vertexCount and vertexBufferID (should be vertexBufferId) aren't prefixed with an underscore as well.
Vertical whitespace between methods/members isn't consistent either. Skip a line between } and the next member, it'll breathe better.

public VertexArray()
    : base()

The : base() part is redundant. Base constructor always gets called.

In the Release() override, you're doing this:

    int id = ID;
    GL.DeleteBuffers(1, ref vertexBufferID);
    GL.DeleteVertexArrays(1, ref id);

Whatever DeleteVertexArrays is doing with id (passed by reference), you're not doing anything with the [possibly] modified value, I'm guessing because ID is readonly. But this is clearly a case where the "why" would be nicely explained with a short comment.

Lastly, properties like DrawMode that use a non-readonly backing field...

DrawMode _drawMode;
public DrawMode DrawMode
{
    get { return _drawMode; }
    set { _drawMode = value; }
}

...would probably be better off rewritten as auto-properties:

public DrawMode DrawMode { get; set; }
public BufferUsageHint BufferUsag { get; set; }
public int VertexCount { get; private set; }


Transforming the case of text using Excel VBA I created a simple user form for my VBA course.

I then created the following VBA code:

Private Sub btnModify_Click()

Dim modRange As Range
Set modRange = Selection

Dim modString As String
modString = modRange.Value

    If obUpperCase.Value = True Then

        modRange.Value = UCase(modString)

        btnModify.Value = False

    ElseIf obLowerCase.Value = True Then

        modRange.Value = LCase(modString)

        btnModify.Value = False

    ElseIf obProperCase.Value = True Then

        modRange.Value = Application.Proper(modString)

        btnModify.Value = False

    End If

End Sub

Is it possible to split the btnModify_Click into two procedures?

The first procedure would check which OptionButton is clicked and return a "formatting" type.

The second would be the actual btnModify_Click. It would take the active selection and apply the formatting type returned from the first procedure.

My apologies for not posting any code along these lines but I don't have a grasp of how procedures work yet. I have an idea but this seems like a simple enough example to get me started (perhaps too simple?).

On a minor note, I don't like the style of the buttons. They have a classic look, and even though there is a time an place for this specific style, is there any way to obtain "better" looking buttons?

First, I'd get rid of the modify button. You can use option button Click events to handle the change.  Something like this:

Private Sub opbLower_Click()
    ActiveCell = LCase(ActiveCell)
End Sub

Private Sub opbProper_Click()
    ActiveCell = Application.Proper(ActiveCell)
End Sub

Private Sub opbUpper_Click()
    ActiveCell = UCase(ActiveCell)
End Sub

To answer your question, you could also use the click events to return a format type for your btnmodify_click method to handle by creating a non local variable above all the code like this:

Dim format As String

Private Sub opbLower_Click()
    format = "lcase"
End Sub

Private Sub opbProper_Click()
    format = "ucase"
End Sub

Private Sub opbUpper_Click()
    format = "pcase"
End Sub

Then apply the condition in the btnmodify_click method based on the value of format.  The first example is better though, it involves less work.


Improved minimal webcrawler - why is it so slow? I recently made a webcrawler that I submitted here for a review:

Minimal webcrawler - bad structure and error handling?

With that help, I've made a much cleaner and better(?) webcrawler.

The only problem I have is that this crawler is extremely slow compared to the other one (13.2s for a depth of 5, compared to 1.3s on the old solution).

Why is it so slow?
With cProfile I have gotten this output over the 10 most time consuming functions. With depth 5, 57 links are crawled or scheduled for crawling - while is_valid_link are called 243 times. Could there be some errors in that method?

from bs4 import BeautifulSoup
import httplib
import urlparse
import urllib2
import time

def sanitize(url, parent):
    if is_internal_link(url):
        if parent.endswith("/") or url.startswith("/"):
            return parent + url
        else:
            return parent + "/" + url
    return url

def is_internal_link(url):
    if url.startswith("http"):
        return False
    return True

def hostname(link):
    hostname = urlparse.urlparse(link).hostname
    return hostname

def get_server_status_code(link):
    # http://stackoverflow.com/questions/1140661
    host, path = urlparse.urlparse(link)[1:3]
    try:
        conn = httplib.HTTPConnection(host)
        conn.request('HEAD', path)
        return conn.getresponse().status
    except StandardError:
        return None

def is_valid_link(link):
    link = sanitize(link, hostname(urlsToCrawl_Parent[0]))
    # http://stackoverflow.com/questions/2924422
    good_codes = [httplib.OK, httplib.FOUND, httplib.MOVED_PERMANENTLY]
    return get_server_status_code(link) in good_codes

def fetch_webpage(url):
    webpage = urllib2.urlopen(url)
    return webpage

def fetch_links(webpage):
    html = webpage.read()
    soup = BeautifulSoup(html)
    links = soup.find_all("a", href=True)
    return links

def schedule_link(link):
    if is_valid_link(link):
        if hostname(link) in urlsToCrawl_Parent[0]:
            urlsToCrawl_Parent.append(link)
    else:
        urlsToCrawl_Child.append(link)

def havent_visited(link):
    if link not in urls_Crawled and link not in urlsToCrawl_Parent and link not in urlsToCrawl_Child:
        return True
    else:
        return False

def crawl(url):
    webpage = fetch_webpage(url)
    links = fetch_links(webpage)
    for link in links:
        link = link['href']
        #This is not done on a good way at all, should clearly do this in fetch_links?
        if havent_visited(link):
            schedule_link(link)

if __name__ == "__main__":
    urls_Crawled = []
    urlsToCrawl_Parent = []
    urlsToCrawl_Child = []

    urlsToCrawl_Parent.append("STARTLINK")
    while (len(urls_Crawled) < 5):
        if (len(urlsToCrawl_Parent) > 0):
            crawl(urlsToCrawl_Parent[0])
            urls_Crawled.append(urlsToCrawl_Parent[0])
            urlsToCrawl_Parent.pop(0)

        elif (len(urlsToCrawl_Child) > 0):
            urlsToCrawl_Parent.append(urlsToCrawl_Child[0])
            crawl(urlsToCrawl_Parent[0])
            urls_Crawled.append(urlsToCrawl_Parent[0])
            urlsToCrawl_Parent.pop(0)

Edit:

Analysis of posted profile results

Of all the functions listed in the result, your code accounts for 0.041 seconds. However, the cumulative time for the entire execution is 14.169. This has your code accounting for .289% of the entire execution.

The function that takes the most time is the recv() calls on the underlying sockets. This one function is dominating the execution time compared to any of the other functions. From this we see that transferring files over the internet is very slow. Evaluating a boolean expression and manipulating a list is very fast.

Based on the analysis, we have learned that your program's bottleneck is the IO over the network. Every time your code reads a new html file, nothing else is happening in your application. What you can do to speed this is up is to execute code in parallel so that you a reading in multiple files at one time. You can look into threads to do this type of parallelization. However, by doing this you will also have to synchronize access ToCrawl and Crawled lists.

Original:

A review of the code independent of an performance considerations.

def hostname(link):
    hostname = urlparse.urlparse(link).hostname
    return hostname

Your variable is hiding the function name. Doing this in a function that is meant to be recursive would lead to an error. Instead you can return the value directly instead of creating a local variable.

There are a few other places you create a local variable and only reference them once. Sometimes giving a value a name can increase readability. However, most of the code in this post do not fit into this category.

def is_internal_link(url):
    if url.startswith("http"):
        return False
    return True

startswith() is already returning a boolean. You don't need the if statement to explicitly return the result.

def is_internal_link(url):
    return not url.startswith("http")

Another example if using an if statement to explicitly return a boolean.

def havent_visited(link):
    if link not in urls_Crawled and link not in urlsToCrawl_Parent and link not in urlsToCrawl_Child:
        return True
    else:
        return False

I point this out because it is done in a different style. This explicitly uses an else where as the first case used the implicit else as the true clause will always exit the function. Using a standard convention throughout the code base will improve readability. In this case, one style is not better, but you should be consistent.

crawl(urlsToCrawl_Parent[0])
urls_Crawled.append(urlsToCrawl_Parent[0])
urlsToCrawl_Parent.pop(0)

is equivalent to

target = urlsToCrawl_Parent.pop(0)
crawl(target)
urls_Crawled.append(target)

The second version is cleaner because the variable name gives you context. You also don't have to worry about indexing the list multiple times.

The elif block is also executing this same operation. You can more this repeated code into a function.

while (len(urls_Crawled) < 5):

5 is a magic number here. There is no context to why a number less than 5 is important. If there is another 5 somewhere else in the code, does it mean the same thing? Giving a constant a name will provide this context. Also, if you need to change it later, you will just have to change the code in one place. And you won't have to worry about that other 5 that had a different context.


Presenting a time as a string I have three methods that are really similar but do slightly different things. This doesn't feel very DRY to me, but I can't think of a way to reduce the duplication. Here are the methods, I think they're fairly self explanatory:

# Present a date as a string, optionally without a year
# Example output: "December 1, 2011"
def date_string(year=true)
  if date
    str = "%B %e"
    str += ", %Y" if year
    date.strftime(str).gsub('  ',' ')
  else
    ""
  end
end

# Present a time as a string, optionally with a meridian (am/pm)
# Example output: "1:30 PM"
def start_time_string(meridian=true)
  if start_time
    str = "%l:%M"
    str += " %p" if meridian
    start_time.strftime(str).lstrip
  else
    ""
  end
end

# Present a time as a string, optionally with a meridian (am/pm)
# Example Output: "2:00"
def end_time_string(meridian=true)
  if end_time
    str = "%l:%M"
    str += " %p" if meridian
    end_time.strftime(str).lstrip
  else
    ""
  end
end

Each method just presents a time as a string with certain options, and if the time object they're trying to present is nil, they return an empty string.

Any ideas how to DRY this up?

Well, I don't see much you can do with the first method (being as it has a different responsibility than the other two). You can just combine the latter two methods into one and make the time object a parameter, like such:

def time_string(time_obj, meridian=true)
  if time_obj
    str = "%l:%M"
    str += " %p" if meridian
    time_obj.strftime(str).lstrip
  else
    ""
  end
end

This should reduce complexity some, since both latter methods have essentially the same responsibility.


Generically call static methods I currently have two (core) classes :

public abstract class BO<TConfig> where TConfig : BOConfigBase
{
    protected TConfig Config { get; set; }
    internal List<BC> _BCs { get; set; }

    public abstract void Init();

    public void Process() => _BCs.ForEach(bc => bc.Process());
}

public class BOOne : BO<BOConfigOne>
{
    public BOOne(BOConfigOne config)
    {
        Config = config;
    }

    public override void Init()
    {
        _BCs = new List<BC>()
        {
            BCA.Init(Config),
            BCB.Init(Config),
            BCC.Init(Config),
            BCOneA.Init(Config)
        };
    }
}

Then the code for my BCs

public abstract class BC
{
    protected BOConfigBase Config { get; set; }
    public abstract void Process();
}

public class BCA : BC
{
    private BCA()
    {
    }

    public static BCA Init(BOConfigBase config)
    {
        BCA ret = new BCA { Config = config };
        return ret;
    }

    public override void Process()
    {
        Config.Counter += 1;
    }
}

To call this, I will do this :

    static void Main()
    {
        {
            var boConfigOne = new BOConfigOne()
            {
                A = "one",
                B = "one",
                C = "one",
                One = "one"
            };
            var testBO = new BOOne(boConfigOne);
            testBO.Init();
            testBO.Process();
            Console.WriteLine(
                $"A = {boConfigOne.A}, B = {boConfigOne.B}, C = {boConfigOne.C}, One = {boConfigOne.One}, Counter = {boConfigOne.Counter}");
        }

        {
            var boConfigTwo = new BOConfigTwo()
            {
                A = "two",
                B = "two",
                C = "two",
                Two = "two"
            };

            var testBOTwo = new BOTwo(boConfigTwo);
            testBOTwo.Init();
            testBOTwo.Process();
            Console.WriteLine(
                $"A = {boConfigTwo.A}, B = {boConfigTwo.B}, C = {boConfigTwo.C}, Two = {boConfigTwo.Two}, Counter = {boConfigTwo.Counter}");
        }
        Console.ReadKey();
    }

BO stands for Business Orchestration, and BC for Business Component. A BC would perform a single function, and a BO would contain several of these re-usable BCs.

I would like to change the BO to be able to Init all the BCs generically, something like Init => BCs.Foreach(bc => bc.Init(Config));. The problem is that my BC's Init's are static, hence I can't put it in an interface, and call it on the interface, nor can I put it in the base abstract method, and override it.

Does anyone have a better solution for me?

To be honest it's hard to follow with such abstract class names and POC.  The root of what you are describing is you have a factory method to create one of your classes.  Lucky for you it seems all the factory methods have the same method signature.  We can make this a bit easier.

Need to store the factory methods in a private field to be used by the Init method.  Also adding a "helper" method to make storing the factories easier.

public abstract class BO<TConfig> where TConfig : BOConfigBase
{
    protected TConfig Config { get; set; }
    internal List<BC> _BCs { get; set; }
    private Func<BOConfigBase, BC>[] _factories = new Func<BOConfigBase, BC>[0];

    protected void SetFactories(params Func<BOConfigBase, BC>[] factories)
    {
        _factories = factories;
    }

    public void Init()
    {
        _BCs = _factories.Select(b => b(Config)).ToList();
    }

    public void Process() => _BCs.ForEach(bc => bc.Process());
}

Now BOOne class can call it like

public class BOOne : BO<BOConfigOne>
{
    public BOOne(BOConfigOne config)
    {
        Config = config;
        SetFactories(BCA.Init, BCB.Init, BCC.Init, BCOneA.Init);
    }
}

I'm assuming each BO class would take different BC classes if not then you could constructer inject all the factories.


The best way to unwrap the optional in java Instead of using the null checks I tried using Optional and safely unwrapping the optional.

Approach 1

My first approach of unwrapping the Optional is using map() followed by orElseThrow().

Approach 2

My second approach if unwrapping the Optional is using orElseThrow() and setting the value to a variable and using it.

Service

@Inject
private UserConfigurationRepository userConfigurationRepository;

public UserConfiguration updateConfigUsingOrElseThrow(User user, String value) {
    UUID configurationId = user.getConfigurationId().orElseThrow();
    UserConfiguration userConfiguration = userConfigurationRepository
            .findById(configurationId).orElseThrow();
    return updateUserConfiguration(userConfiguration, value); // Performs update operation
}

public UserConfiguration updateConfigUsingMapOrElseThrow(User user, String value) {
    return user.getConfigurationId().map(configurationId ->{
        return userConfigurationRepository.findById(configurationId).map(userConfiguration -> {
            return updateUserConfiguration(userConfiguration, value); // Performs update operation
        }).orElseThrow();
    }).orElseThrow();
}

private UserConfiguration updateUserConfiguration(UserConfiguration userConfiguration, String value)
{
    userConfiguration.setValue(value);
    // Business Logics
    return userConfigurationRepository.save(userConfiguration);
}

Repository

public interface UserConfigurationRepository extends JpaRepository<UserConfiguration, UUID>{}

Model

public class User {
   private int id;
   private String name;
   private String email;
   private UUID configurationId;

   public Optional<UUID> getConfigurationId() {
       return Optional.ofNullable(configurationId);
   }

   public void setConfigurationId(UUID configurationId) {
       this.configurationId = configurationId;
   }

   //Ignored getters and setters of other properties those are not used in this part of code
}

public class UserConfiguration {
   private UUID id;
   private String value;

   public UUID getId() {
       return id;
   }

   public void setId(UUID id) {
       this.id = id;
   }

   public String getValue() {
       return value;
   }

   public void setValue(String value) {
       this.value = value;
   }
}

I am used to using null check using if statement and since I am new to this style of programming I am not sure which one is the best approach and gives more readability, Or is there any other recommended way to do this?

The task of this service is to get the configuration id and a configuration message from front end and update the data in the configuration table. And returns the updated configuration information. Not all the users will have an entry in the configuration so there is a possibility of getting null value for configurationId in the User model thats is why the getter returns the optional of configurationId.

Since this is my client code I am unable to expose the complete code, So I provided the structure of the model, services and repository. Hope I have provided necessary information.

I think you want flatMap.
Those returns and braces are unnecessary noise.
I would consider not throwing but returning Optional<UserConfiguration> instead. Part of learning this style is realising that unwrapping is rarely necessary. At the call site, you can do something along the lines of update(user).ifPresentOrElse(...). [*]

public Optional<UserConfiguration> update(User user) {
    return user.getConfigurationId() // Optional<UUID>
        .flatMap(configurationId -> userConfigurationRepository.findById(configurationId)) // Optional<UserConfiguration>
        .map(userConfiguration -> updateUserConfiguration(userConfiguration)); // Optional<UserConfiguration>
}

However, if you insist on unwrapping, nothing's stopping you, technically.

public UserConfiguration update(User user) {
    return user.getConfigurationId() // Optional<UUID>
        .flatMap(configurationId -> userConfigurationRepository.findById(configurationId)) // Optional<UserConfiguration>
        .map(userConfiguration -> updateUserConfiguration(userConfiguration)) // Optional<UserConfiguration>
        .orElseThrow();
}

consider method references. Sometimes they make code more readable, other times they don't. But mostly yes.

public Optional<UserConfiguration> update(User user) {
    return user.getConfigurationId() // Optional<UUID>
        .flatMap(userConfigurationRepository::findById) // Optional<UserConfiguration>
        .map(this::updateUserConfiguration); // Optional<UserConfiguration>
}

[*] - Getting into this mindset would help you a lot when you will start working with other wrapper types that rely on the same abstractions, such as the standard library's Stream, RxJava, Spring 5-reactive stack / Project Reactor, or classes provided by the vavr library. Although possible, unwrapping some of these is considered a very bad idea, e.g. in case of RxJava and Reactor it is a blocking action which you want to avoid at all costs when writing reactive software. But even in the case of Optional, why not have the fact that the value might be missing captured by the type system rather that throwing Exceptions around that might be invisible up the stack until there is actually an issue.


Finding largest number we can make The problem is from yesterday's contest from Codeforces.

I have observed that, from the left side (i.e. MSB), for every digit, if we replace the given number with the output of f when after passing the digit - we get a bigger number we will get the desired output. I'm a beginner in programming and have written code with Python 3, which is showing TLE after submission (though it is correct and gives correct output for the sample test cases). I think a better algorithm is needed.

Variables st (start index), flag, cntr (length of contiguous subsegment ) are taken for the condition the part where we will replace the digits needs to be contiguous subsegment:

n = int(input())
s = input()
f = list(map(int,input().split()))
result = []
st = -1
flag = 1
cntr = 0
for i in range(len(s)):
    x=f[int(s[i])-1]
    if(x>int(s[i])):
        result.append(str(x))
        if(flag==1):
            st = i
            flag = 0
        if(flag==0):
            cntr+=1
    else:
        result.append(s[i])
        if(st!=-1 and (i-st-1)!=cntr):
            break
for j in range(i+1,len(s)):
    result.append(s[j])
#print(result)
r = int(''.join(result))
print(r)

Can anyone make any improvement in my code? I know there is a tutorial on that website, but I want to learn how to develop code which I have written already.

The speed issue can be fixed by using PyPy 3  as it is faster (at least in most codeforces type problems, I don't know in general)

However this will result in a WA verdict. To fix this just modify the break condition as follows and it will work:

if(st!=-1 and x!=int(s[i])):

This is because the current version can sometimes break out of the loop prematurely when \$x == int(s[i])\$ because it may be better to make the segment longer.

Finally, I consider you are using too many flag variables, only one is required. Here is how I would change the code:

n = int(input())
s = input()
f = list(map(int,input().split()))
result = []
flag = 1
for i in range(len(s)):
    x=f[int(s[i])-1]
    if(x>int(s[i])):
        result.append(str(x))
        if(flag==1):
            flag = 0
    else:
        result.append(s[i])
        if(flag!=1 and x!=int(s[i])):
            break
for j in range(i+1,len(s)):
    result.append(s[j])
#print(result)
r = int(''.join(result))
print(r)


Using the RAII design pattern in VBA to automatically restore settings Using a technique described in this StackOverflow answer by @David Murdoch I created a custom class with methods to speed up Excel (Activate turns off screen updates and set the calculation mode to manual) and to restore it to normal settings (Shutdown).  When the object reaches the end of its life and Class_Terminate is triggered it runs the Shutdown procedure.

The purpose of all this is to avoid problems that occur if (when) I forget to reset these settings at the end of a script or if my code (contains a bug that) skips the section resetting these settings.

I am looking for a broad critique of the nit-picky variety.  In other words, how you would code this.

This code intentionally ignores the initial state of the settings and always turns on screen updating and sets the calculation mode to automatic when the object is destroyed.

The following is in a class module called ExcelTurboBooster.

Option Explicit
'
'  Use the code below to turn off screen updates and automatic
'   calculations.  Screen updates and automatic calculations will be
'   restored when the calling procedure terminates.
'
' EXAMPLE USAGE
' ======================================================================
'   Dim objExcelTurboBooster As New ExcelTurboBooster
'   objExcelTurboBooster.Activate   ' Activates booster
'   objExcelTurboBooster.Shutdown   ' Use to deactivate before procedure
'                                   '  terminates
' ======================================================================
'
'
Public Sub Activate()
    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
End Sub

Public Sub Shutdown()
    On Error Resume Next
    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = True
End Sub

Private Sub Class_Terminate()
    Shutdown
End Sub

I would point out that the naming convention is inconsistent -- we have Activate and a Shutdown --- they don't say the same thing to me. I'd expect corresponding actions to have names like Activate -> Deactivate, Enable -> Disable, Startup -> Shutdown. Given the nature of the class, Enable/Disable pair seems the best suited for what your class is going to do, since it's basically toggling an application level state.

Your Shutdown has an On Error Resume Next but says nothing if it was successful. I would at least expect the procedure to exit with something like ShutDown = (Err.Number = 0) so that the calling client can at least know there was a clean exit or not. However, I'm more inclined to encourage to rethink whether you really want a Resume Next there, because you are dealing with altering the application's state, so it is crucial to know that the alterations were successful so that the clients can proceed with the assumption that they are in the correct state.

The other thing that occurs to me is that you probably don't actually need the methods at all. You only need the Class_Initialize and Class_Terminate. Your calling code then becomes...

Set booster = New ExcelTurboBooster
'Do lot of complicated stuff....
Set booster = Nothing

Look, ma, no methods! In fact, technically you only need the first line. The last line is superficial because when your procedure that instantiate the object exits (even if it exited badly), the instance will go poof and the application state should be reset. The biggest downside of that method, though, is that error handling becomes somehow strange inside the Initialize/Terminate events, so it could be difficult to communicate to your clients there has been a problem.

The downside with the no-method approach is that it totally can't go into a public field of any standard module or maybe long-lived classes either because then they will persist too much longer. One'd have to take discipline to use it only as a local variable inside procedure so that it will be destroyed at the end of the procedure. Otherwise, you've built up a new layer of complication for nothing.


Merge Sort Java Implementation Can someone check my Merge Sort implementation on Java? Is it good/bad and can it be improved further?

public class MyMergeSort {
    private int [] data;

    public MyMergeSort(int[] data){
        this.data = data;
    }

    public int[] sort(int[] data){
        if (data.length < 2){
            return data;
        }
        int mid = data.length/2;
        int first[] = new int[mid];
        int second[] = new int[data.length - mid];
        System.arraycopy(data, 0, first, 0, mid);
        System.arraycopy(data, mid, second, 0, data.length - mid);
        return merge(sort(first), sort(second));
    }

    private int[] merge(int[] arr1, int[] arr2) {
        if(arr1.length == 0){
            return arr2;
        }
        if(arr2.length == 0){
            return arr1;
        }
        int res[] = new int[arr1.length + arr2.length];
        int i;
        int j = 0; // iterate over the first array
        int k  = 0; // iterate over the second array
        for (i = 0; i < res.length; i++) {
            if(j < arr1.length && k < arr2.length){
                if(arr1[j] < arr2[k]){
                    res[i] = arr1[j];
                    j++;
                }
                else{
                    res[i] = arr2[k];
                    k++;
                }
            }
            else{
                if(j < arr1.length){
                    System.arraycopy(arr1, j, res, i, arr1.length-j);
                    break;
                }
                else{
                    System.arraycopy(arr2, k, res, i, arr2.length-k);
                    break;
                }
            }
        }
        return res;
    }

    public static void main(String[] args) {
        int d[] = {1, 9, 2, 8, 6, 3, 7, 4, 2};
        MyMergeSort mySort = new MyMergeSort(d);
        int res[] = new int[d.length];
        res =  mySort.sort(d);
        for (int i = 0; i < res.length; i++) {
            System.out.print(res[i] + " ");
        }
    }
}

Be consistent with your formatting. Sometimes you have a space between keywords like if and else and the opening parenthesis and the closing parenthesis and the opening brace, sometimes not. Pick one (I prefer a space) and stick with it. I also put spaces around all binary operators: data.length / 2 to match i < res.length. Again, consistency helps readability.
The data passed to the constructor is ignored; remove it.
Can you come up with more readable names for arr1 and arr2? Perhaps left and right (even though that may be misleading and is still arbitrary) will be easier to read than spotting 1 vs. 2 in a name.
In merge I think using while (j < arr1.length && k < arr2.length) instead of for is more readable. It also allows you to move the "copy the rest of the non-empty array onto the end" logic out of the loop and avoid using break.
Don't create a result array in main before calling sort--it's thrown away immediately.


Scoring Ping Pong in Elixir I have some elixir code I am using to score a ping pong game. Since I am new to functional programming I was hoping there might be a more elegant solution to this.

This part of the code is used in a GenServer to check if the next point could result in a win. So that means will the leading players next point meet the minimum score requirement (11 or 21 in ping pong) and also be 2 points in front of the other play.

Anyone got a better idea here?

def handle_call(:next_point_wins, _from, game) do
#game.score = [10,5]
nextPlayer1 = List.first(game.score) + 1
nextPlayer2 = List.last(game.score) + 1

scoreDifference =
  case nextPlayer1 > nextPlayer2 do
    true
      ->
      leader = nextPlayer1
      nextPlayer1 - nextPlayer2
    false
      ->
      leader = nextPlayer2
      nextPlayer2 - nextPlayer1
  end

cond do
  scoreDifference + 1 >= 2 and leader >= game.winningScore
    ->
      IO.puts "SOMEONE IS ABOUT TO WIN"
      {:reply, :ok, game}
  true
    ->
      IO.puts "NO-ONE IS ABOUT TO WIN"
      {:reply, :ok, game}
end

end

There's a few things you could do to improve this code:

Don't assign inside a case statement.

You assign a value to leader inside this case statement:

scoreDifference =
  case nextPlayer1 > nextPlayer2 do
    true
      ->
      leader = nextPlayer1
      nextPlayer1 - nextPlayer2
    false
      ->
      leader = nextPlayer2
      nextPlayer2 - nextPlayer1
  end

Support for this is deprecated in Elixir, and I assume will eventually be removed.  The Elixir compiler will complain about this when it sees it, and you should listen to it.  Generally you should find a different way to express this.  One option is to return a tuple from your case:

{scoreDifference, leader} =
  case nextPlayer1 > nextPlayer2 do
    true ->
      {nextPlayer1 - nextPlayer2, nextPlayer1}
    false ->
      {nextPlayer2 - nextPlayer1, nextPlayer2}
  end

Use if when appropriate.

A cond statement that has one branch with conditions and one true branch is basically just an if statement.  So it'd be simpler to just use an if:

if (scoreDifference + 1) >= 2 and leader >= game.winningScore do
  IO.puts "SOMEONE IS ABOUT TO WIN"
else
  IO.puts "NO-ONE IS ABOUT TO WIN"
end
{:reply, :ok, game}

(I've also moved the reply out of the if statement there, since it was the same across each.  If you wanted to do different things you could move it back in easy enough).

Use Pattern Matching to destructure the scores.

Assuming you are only ever expecting 2 entries in your score list, you could use pattern matching to extract the two scores.

[nextPlayer1, nextPlayer2] = game.score

I'm not adding 1 to the scores here, because this isn't neccesary to calculate the difference correctly. (It is neccesary to add one to check if someone has a winning score, but I'll come to that).

Simplifying the logic

You can just subtract one score from the other and then call abs on the result to get a positive number for the score difference:

scoreDifference = abs(nextPlayer1 - nextPlayer2)

Then you can find the leader with Enum.max:

leader = Enum.max(game.score)

Putting it all together

If you take all these improvements you might end up with something like:

[nextPlayer1, nextPlayer2] = game.score
scoreDifference = abs(nextPlayer1 - nextPlayer2)
leaderScore = Enum.max(game.score)
if scoreDifference + 1 >= 2 and (leaderScore + 1) >= game.winningScore do
  IO.puts "SOMEONE IS ABOUT TO WIN"
else
  IO.puts "NO-ONE IS ABOUT TO WIN"
end
{:reply, :ok, game}