Python reimplementation of Lost In Space by Tim Hartnell In a university library, there was copy of "Making The Most Of Your ZX 81" by Tim Hartnell. I flipped to an interesting page from the index and found this program in BASIC, and thought it interesting enough to rewrite in Python. It's a number guessing game.

from random import random
from sys import exit
print("LOST IN SPACE:\nYou have fifteen hours to find a capsule lost in a SEVEN kilometer cube of space.\n")
def main():
    def winCondition(i):
            print("You found it with {} hours of air left".format(15 - i))
            end()
    def end():
        print("Play again? Y/N")
        response = input()
        if response.lower() == "y":
            game()
        else:
            exit(0)
    def game():
        A = int(7 * random() + 1)
        B = int(7 * random() + 1)
        C = int(7 * random() + 1)
        """
        Ripped from https://stackoverflow.com/questions/57813858/iteratively-assigning-variables-in-python/57813901#57813901
        """
        def get_input(axis):
            while True:
                user_input = input(f"{axis}-Axis: ")
                try:
                    axis_int = int(user_input)
                    assert(axis_int <= 7)
                except:
                    print("Enter a natural number in the range!")
                    continue
                else:
                    break
            return axis_int
        for i in range(15):
            print("Input 3 search coordinates")
            D, E, F = [get_input(axis) for axis in "YXZ"]
            print(str(D) + str(E) + str(F))
            if A == D and B == E and F == C:
                winCondition(i)
            else:
                print("You have {} hours of air left.".format(15 - i))
                if A > D:
                    print("UP")
                elif A < D:
                    print("DOWN")
                if B > E:
                    print("PORT")
                elif B < E:
                    print("STARBOARD")
                if C > F:
                    print("FORWARD")
                elif C < F:
                    print("BACKWARD")
                if i == 14:
                    print("Choose your next move carefully... HYPOXIA IMMINENT.")
        print("Fail, astronaut dead; capsule was at coords {}.{}.{}".format(A, B, C))
        end()
main()

I first of all want to improve concision before anything else.

Nested functions

Not really sure what your motivation was, here, but this Russian nesting doll of functions is not really necessary. Under certain narrow circumstances, like limiting the number of publicly visible methods in a library, it might be called-for - but not really here. You don't need closures for this code, so just move things up to global namespace.

Variable case

A-C should be lower-case because they aren't types.

Random unpacking

a, b, c = (int(7*random() + 1) for _ in range(3))

Never except:

Currently, you can't Ctrl+C out of the program, because you're catching all exceptions. Narrow your exception type.

Don't materialize generators

D, E, F = [get_input(axis) for axis in "YXZ"]

should be

d, e, f = (get_input(axis) for axis in "YXZ")

because you don't need to keep that list in memory.

Format strings

print(str(D) + str(E) + str(F))

becomes

print(f'{d}{e}{f}')

Stack abuse

You call game, which calls end, which calls game... Don't do this. Just loop.


PowerShell script to read line by line large CSV files I am managing large CSV files (files ranging from 750 Mb to 10+ Gb), parsing their data into PSObjects, then processing each of those objects based on what is required.

I wrote the following script to churn through these files line by line, filter based on one of the data fields, then close the file.  The script works but I feel that it could be faster.  For instance, it took 4.5 hours to parse a 389k line csv file. Taking that filesize and timeline, it would take over two and a half days to work through just the sorting and filtering of the data!

Before anyone suggests "use a database!", I'm 100% with you and have started the RFC for adding a database server to our network.  Unfortunately our CAB only meets quarterly and this wasn't deemed an emergency.  So, I'm left without a database solution for at least 2-3 months.

Anyhow, here is the code:

[void][reflection.assembly]::LoadWithPartialName("Microsoft.VisualBasic")

$source = Get-FileName "C:\users\$env.username\Downloads"

$reader = New-Object Microsoft.VisualBasic.FileIO.TextFieldParser $source

$reader.SetDelimiters(",")

While(!$reader.EndOfData)
{
    $line = $reader.ReadFields()

    $details = [ordered]@{
                            "Plugin ID" = $line[0]
                            CVSS = $line[2]
                            Risk = $line[3]
                            Host = $line[4]
                            Protocol = $line[5]
                            Port = $line[6]
                            Name = $line[7]
                            Description = $line[9]
                            Solution = $line[10]
                            "Plugin Output" = $line[12]
                         }

    $pluginID = $line[0]
    $risk = $line[3]

    if ($risk -eq "Critical" -or $risk -eq "High" -or $risk -eq "Medium" -or $risk -eq "Low")
    {
        $allVulns += New-Object PSObject -Property $details
    }
    else
    {
        # Filters data into objects based on their plugin ID
        Switch ($pluginID)
        {
            11936 # OS Identification
            {
                $11936 += New-Object PSObject -Property $details
                break
            }
            14272 # Open Ports
            {
                $14272 += New-Object PSObject -Property $details
                break
            }
            20811 # Software Inventory
            {
                $20811 += New-Object PSObject -Property $details
                break
            }
            54615 # Device Type
            {
                $54615 += New-Object PSObject -Property $details
                break
            }
            66334 # Missing OS Patches
            {
                $66334 += New-Object PSObject -Property $details
                break
            }
        }
    }
}

# Close the read file
$reader.Close()

Is there a faster, more efficient way for this code to execute?

Thanks!

EDIT: Some sample data by request! This is two of the plugins I'm parsing that don't have endless amounts of Plugin Output data.

Plugin ID,CVE,CVSS,Risk,Host,Protocol,Port,Name,Synopsis,Description,Solution,See Also,Plugin Output
11936,,,None,GCAB-L7-449096R,tcp,0,OS Identification,It is possible to guess the remote operating system.,"Using a combination of remote probes (e.g., TCP/IP, SMB, HTTP, NTP,
SNMP, etc.), it is possible to guess the name of the remote operating
system in use. It is also possible sometimes to guess the version of
the operating system.",n/a,,"
Remote operating system : Microsoft Windows 7 Enterprise Service Pack 1
Confidence level : 100
Method : SMB

The remote host is running Microsoft Windows 7 Enterprise Service Pack 1"
14272,,,None,GCAB-L7-449096R,udp,53125,netstat portscanner (SSH),Remote open ports are enumerated via SSH.,"This plugin runs 'netstat' on the remote machine to enumerate open
ports.

See the section 'plugins options' about configuring this plugin.",n/a,https://en.wikipedia.org/wiki/Netstat,Port 53125/udp was found to be open

It might be a little hard to make a suggestion about this as I cannot see how the data is being used beyond this. However I can point out the large performance flaw in your logic.
Building arrays with +=
You have multiple lines of code that go like this:
$20811 += New-Object PSObject -Property $details

What is actually happening there is PowerShell is destroying the array $20811 and creating a new one that is one element larger to house the data on the right hand side. For small operations this performance hit is negligible. However you will certainly feel it when you get into the thousands of elements. You are rebuilding new arrays with every operation.
Consider array lists
I would instead just create all of the custom objects in one pass into one large variable instead. Then you could use Where-Object to process the groups of rows as you see fit. Or you can still keep them as separate objects. Either way I would use an arraylist instead.
$11936 = New-Object System.Collections.ArrayList

# truncated ....
11936 # OS Identification
{
    [void]($11936.Add($details))
    break
}

The [void] cast suppresses the output created from the Add method
This should be significantly faster.
Read the CSV as a database
There are methods to read the CSV as a database as well. The one I tested was using the Microsoft.ACE.OLEDB.12.0 provider. This method allows you to use SQL statements against the CSV file. I don't really have the time to properly benchmark this but this should be faster than your current method as well. Consider a simple example using mock "employee" data:
@"
id,first_name,last_name,email,ip_address
1,Edward,Richards,erichards0@businessweek.com,201.133.112.30
2,Jimmy,Scott,jscott1@clickbank.net,103.231.149.144
3,Marilyn,Williams,mwilliams2@chicagotribune.com,52.180.157.43
4,Frank,Morales,fmorales3@google.ru,218.175.165.205
5,Chris,Watson,cwatson4@ed.gov,75.251.1.149
6,Albert,Ross,aross5@abc.net.au,89.56.133.54
7,Diane,Daniels,ddaniels6@washingtonpost.com,197.156.129.45
8,Nancy,Carter,ncarter7@surveymonkey.com,75.162.65.142
9,John,Kennedy,jkennedy8@tumblr.com,85.35.177.235
10,Bonnie,Bradley,bbradley9@dagondesign.com,255.67.106.193
"@ | Set-Content .\test.csv

$conn = New-Object System.Data.OleDb.OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source='C:\Users\Cameron';Extended Properties='Text;HDR=Yes;FMT=Delimited';")
$cmd=$conn.CreateCommand()
$cmd.CommandText="Select * from test.csv where first_name like '%n%'"
$conn.open()
$data = $cmd.ExecuteReader()

$data | ForEach-Object{
    [pscustomobject]@{
        id=$_.GetValue(0)
        first_name=$_.GetValue(1)
        last_name=$_.GetValue(2)
        ip_address=$_.GetValue(4)
    }
}

$cmd.Dispose()
$conn.Dispose()

I have a SQL statement in there that will return all the records where the first name has a "n" in it. Using the field indecies we build a custom psobject that gets sent down the pipe.
Note that the source in the connection string is the folder that contains the csv file. And the table in the SQL statement is the CSV file name.


Swift function to interpret Roman numerals (ported from JavaScript) I have written some Swift for the first time, being competent enough in Javascript and having some experience in Ruby and Python.  For my education, I've written a function that parses a roman numeral string and returns its integer representation, first in Javascript (ES2015+):

const dict = [
  ['CM', 900], ['M', 1000], ['CD', 400], ['D', 500],
  ['XC', 90], ['C', 100], ['XL', 40], ['L', 50],
  ['IX', 9], ['X', 10], ['IV', 4], ['V', 5],
  ['I', 1],
]

function romanToInt (original) {
  let temp = original
  let int = 0

  while (temp.length > 0) {
    let found = false

    for (const [glyph, quantity] of dict) {
      if (temp.startsWith(glyph)) {
        int += quantity
        temp = temp.slice(glyph.length)
        found = true
        break
      }
    }

    if (!found) {
      // e.g. Error parsing roman numeral "MDCCLXXVI," at ","
      throw new Error(`Error parsing roman numeral "${original}" at "${temp}"`)
    }
  }

  return int
}

try {
  romanToInt('MMXIV') // => 2014
} catch (err) {
  console.error(err)
}

and then ported it to Swift 4:

let dict: [( glyph: String, quantity: Int )] = [
    ("CM", 900), ("M", 1000), ("CD", 400), ("D", 500),
    ("XC", 90), ("C", 100), ("XL", 40), ("L", 50),
    ("IX", 9), ("X", 10), ("IV", 4), ("V", 5),
    ("I", 1)
]

enum RomanNumericError: Error {
    case badInput(original: String, temp: String)
}

func romanToInt(original: String) throws -> Int {
    var temp = original
    var int = 0

    while temp.count > 0 {
        var found = false

        for (glyph, quantity) in dict {
            if temp.starts(with: glyph) {
                int += quantity
                temp.removeFirst(glyph.count)
                found = true
                break
            }
        }

        guard found == true else {
            throw RomanNumericError.badInput(original: original, temp: temp)
        }
    }

    return int
}

do {
    try romanToInt(original: "MMXIV") // => 2014
} catch RomanNumericError.badInput(let original, let temp) {
    print("Error parsing roman numeral '\(original)' at '\(temp)'")
}

I'm wondering about how swift-y my code is in terms of design patterns, especially in terms of error handling. In Javascript, throwing and catching errors is a very common control flow design, and I'm wondering if I'm approaching it from the right angle in Swift.

Let's do this from inside out. temp.starts(with: glyph) is correct, but
there is also a dedicated method temp.hasPrefix(glyph) for strings.

The loop to find the first dictionary entry with a matching prefix
can be shortened to

guard let (glyph, quantity) = dict.first(where: { temp.hasPrefix($0.glyph) }) else {
    throw RomanNumericError.badInput(original: original, temp: temp)
}

(also making the var found obsolete.)

Mutating the temporary string can be avoided by working with a SubString (which is a kind of view into the original string) and
only updating the current search position:

var pos = original.startIndex
while pos != original.endIndex {
    let subString = original[pos...]
    // ...
    pos = original.index(pos, offsetBy: glyph.count)
}

Naming: This is very opinion-based, here are my opinions:

Declare the function as func romanToInt(_ roman: String),
so that it is called without (external) argument name:
romanToInt("MMXIV").
Rename var int to var value.
dict is also a non-descriptive name (and it is not even a dictionary), something like glyphsAndValues might be a better choice.

Summarizing the suggestions so far, we have

func romanToInt(_ roman: String) throws -> Int {
    var value = 0
    var pos = roman.startIndex
    while pos != roman.endIndex {
        let subString = roman[pos...]
        guard let (glyph, quantity) = glyphsAndValues.first(where: { subString.hasPrefix($0.glyph) }) else {
            throw RomanNumericError.badInput(roman: roman, at: subString)
        }
        value += quantity
        pos = roman.index(pos, offsetBy: glyph.count)
    }
    return value
}

Now the error handling. Yes, throwing an error is a good and Swifty
way to report a failure to the caller. (An alternative is to return an
optional value which is nil in the error case, but that does not
allow to provide additional error information.)

The creation of the error message however should be done in the
error class, by adopting the LocalizedError protocol:

enum RomanNumericError: Error {
    case badInput(roman: String, at: Substring)
}

extension RomanNumericError: LocalizedError {
    public var errorDescription: String? {
        switch self {
        case .badInput(let roman, let at):
            return "Error parsing roman numeral '\(roman)' at '\(at)'"
        }
    }
}

The big advantage is that the caller does not need to know which
error the function throws, and can catch a generic Error:

do {
    try print(romanToInt("MMXIV"))
    try print(romanToInt("MMYXIV"))
} catch {
    print(error.localizedDescription)
}

// Output:
//   2014
//   Error parsing roman numeral 'MMYXIV' at 'YXIV'


Thread synchronization with mutex This program prints odd numbers by thread1 and even numbers by thread2 sequentially. Can this code be optimized or made more efficient?

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t condition_var = PTHREAD_COND_INITIALIZER;

void *function_count1();
void *function_count2();

int count = 1;

int main()
{
    pthread_t thread1, thread2;
    int ret;

    if( (ret=pthread_create( &thread1, NULL, &function_count1, "thread1")) )
    {
        printf("Thread 1 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    if( (ret=pthread_create( &thread2, NULL, &function_count2, "thread2")) )
    {
        printf("Thread 2 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    pthread_join( thread1, NULL);
    pthread_join( thread2, NULL);

    exit(EXIT_SUCCESS);
}

void *function_count1(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );
        if (count % 2 != 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock( &count_mutex );
    }
}

void *function_count2(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );

        if (count % 2 == 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock(&count_mutex);
    }
}

Since pthread_create() returns an error number on failure, you can put ret into strerror() to get the associated error message instead of just printing the number:

fprintf(stderr, "Thread 1 creation failed: %s\n", strerror(ret));

Notice that I'm calling fprintf() instead of printf().  Since you're printing an error, it must be sent to stderr, not stdin (used by default with printf()).

Also, while you'd normally not need all this code for printing an error, it's necessary since pthread_create() doesn't set errno (which is why ret is useful here).

For code that does set errno (which would be stated on man pages), you'd only need to call perror() to print the corresponding error message:

perror("<insert source of failure here>");

This would also eliminate the need for variables like ret in such code.  However, with pthread_create() and related functions, you could probably just put errno in its place and then use perror().
The functions called by pthread_create() should still return something.  Since the return value won't be useful here, you can just return NULL.
There's no need to call exit() right at the end of main().  You're preventing it from returning a value, and it'll always return 0 at this point anyway.
This isn't a criticism, but a tip: make sure you're running this through valgrind to check for any memory leaks along the way.

Actually, this may already cause a leak, but I haven't checked to make sure.  If it does, then you may need to call both pthread_mutex_destroy() and pthread_cond_destroy() before exiting the program at any point.


Count the number of digits in an integer using F# and log10 This code counts the number of digits in an integer.

let digitCount number = (int) (log10 ((float)number)) + 1;

For instance, it tells us 123456789 has 9 digits.

let input = 123456789;
printfn "%i has %i digits" input (digitCount input);

https://dotnetfiddle.net/nxuoah

A weakness of the code is that it uses two casts. Can we use math to count the number of digits in an integer without having to cast twice? The reason to cast to a float is to make the type compatible with a the log10 function. The reason to cast back in to an integer is to represent a "count" of digits instead of a "measure" of digits.

A second, related weakness of the code is the number of parentheses, which makes the code more difficult to read.

Your code contains a bug. What happens if number is 0? log10 0 should  return -Infinity, so we end up with the smallest int plus one. Therefore, we need to check whether input is 0 first:

let digitCount number = if number == 0 then 1 else (int) (log10 ((float)number)) + 1;

At that point we're not really succinct anymore, so there's no harm in using a recursive function instead:

let rec digitCount number = if number < 10 then 1 else 1 + digitCount (number / 10)

However, that's not tail recursive, so we would want to rewrite it using a loop that's tail recursive. That's left as an exercise, though.

That being said, one can optimize counting digit a lot.

Andrei Alexandrescu held a talk about optimizing digits_base_10 in C++ several years ago. The original talk is offline, unfortunately, but the slides are still there.


Tic Tac Toe Game in Python 3 I made this as a personal challenge. It seems to work fine as I can't find any bugs and am happy with how it runs, but I am interested in what I should do to make the code more professional.

Note: This code was tested in Python 3.7.4.

class TicTacToe:
    def __init__(self):
        self.p1 = '\U00002B55'
        self.p2 = '\U0000274C'
        self.draw = [0,1,2,3,4,5,6,7,8]
        self.counte = 0
        self.Print()
    def fillNumber(self,inputnumber):
        self.counte = self.counte + 1
        if self.counte % 2 == 0 :
            player = self.p2
        else :
            player = self.p1
        self.draw[inputnumber] = player
    def condition(self):
        if (self.draw[0] == self.draw[1] and self.draw[1] == self.draw[2]) :
            return self.draw[0]
        elif (self.draw[3] == self.draw[4] and self.draw[4] == self.draw[5]) :
            return self.draw[3]
        elif (self.draw[6] == self.draw[7] and self.draw[7] == self.draw[8]) :
            return self.draw[6]
        elif (self.draw[0] == self.draw[3] and self.draw[3] == self.draw[6]) :
            return self.draw[0]
        elif (self.draw[1] == self.draw[4] and self.draw[4] == self.draw[7]) :
            return self.draw[1]
        elif (self.draw[2] == self.draw[5] and self.draw[5] == self.draw[8]) :
            return self.draw[2]
        elif (self.draw[0] == self.draw[4] and self.draw[4] == self.draw[8]) :
            return self.draw[0]
        elif (self.draw[6] == self.draw[4] and self.draw[4] == self.draw[2]) :
            return self.draw[6]
        else:
            return False
    def Print(self):
        for i in range(1,10) :
            print(self.draw[i-1],end=" ")
            if i % 3 == 0 :
                print()

class Game(TicTacToe):
    def start(self):
        try :
            inputnumber = int(input('Input number 0 to 8 \n'))
        except ValueError :
            self.start()
        if 0 <= inputnumber <= 8 :
            if (self.draw[inputnumber] == '\U00002B55') or (self.draw[inputnumber] == '\U0000274C') :
                print("This place is already paused")
                self.start()
            else :
                self.fillNumber(inputnumber)
        else :
            self.start()
        self.Print()
        self.check()
    def check(self):
        if self.condition() == False :
            self.start()
        elif self.condition() == '\U00002B55' :
            print(self.condition()," Player one is winner")
        elif self.condition() == '\U0000274C' :
            print(self.condition()," player two is winner")

if __name__=="__main__":
    obj=Game()
    obj.start()

First of all, I suggest you read pep8 (https://pep8.org/). I see you have violations. Understanding pep8 is a must if you want to be a pro Python developer. Better yet, use flake8 to automatically find pep8 violations in your code.

Second, using that many elif statements is always a red flag, especially when they seem to be doing the same thing. Can you somehow get rid of that duplication?

Next, put some blank lines between logical blocks of code to improve readability. In Game.start the try catch block is logically separate from the conditional following it, for example.

Next, give your variables meaningful names. For example, game.start is much better than obj.start. Also, avoid using shortened version of words, like object -> obj because it doesn't add much value and can decrease readability.

Finally, I'd like to see some tests. Not because I think your code has a bug but because I want to know how your code actually works. Because there's some logical reasoning in the nature of the problem, you can help the reader by documenting the algorithm you've implemented. Unit tests are perfect for this. They're fast, they target specific conditions one at a time, and document your code better than comments.
If you're wondering where to start when it comes to testing your code I suggest watching some videos on TDD in action. Kent Beck and Uncle Bob have some free videos showing how to do TDD step by step.


Printing the most used words from phrases I have a collection of phrases in a List. Each phrase is a String Array where each element in the array is a word.

I create a List<Entry<String, Integer>> which holds the words as keys and the times used as value. Everything is sorted by value in descending order.

Then what I do is print the top X words that were used along with how many times they were used. What I want to know is if there is a better/simpler way of doing it and generally anything you want to add to make my code look or perform better.

Here is my code:

public class WordCounting {

    public static void printTopWords(final int numberOfWords, List<String[]> phrases) {
        List<Entry<String, Integer>> wordsMap = entriesSortedByValues(wordCount(phrases));

        Iterator entries = wordsMap.iterator();
        int wordsCounter = 1;
        while (entries.hasNext() && wordsCounter <= numberOfWords) {
            Entry entry = (Entry) entries.next();
            String key = (String) entry.getKey();
            int value = (int) entry.getValue();

            System.out.println(wordsCounter + ": " + key + " - " + value);
            wordsCounter++;
        }
    }

    private static Map<String, Integer> wordCount(List<String[]> phrases) {
        Map<String, Integer> wordCounter = new TreeMap<>();

        for (String[] strings : phrases) {
            for (String string : strings) {
                wordCounter.put(string, wordCounter.get(string) == null
                        ? 1 : wordCounter.get(string) + 1);
            }
        }
        return wordCounter;
    }

    static <K, V extends Comparable<? super V>>
            List<Entry<K, V>> entriesSortedByValues(Map<K, V> map) {

        List<Entry<K, V>> sortedEntries = new ArrayList<>(map.entrySet());

        Collections.sort(sortedEntries,
                new Comparator<Entry<K, V>>() {
                    @Override
                    public int compare(Entry<K, V> e1, Entry<K, V> e2) {
                        return e2.getValue().compareTo(e1.getValue());
                    }
                }
        );

        return sortedEntries;
    }
}

As others have pointed out the most obvious improvements already, I would like to talk about Java 8. Maybe you cannot use it yet, but I would recommend looking into it and this answer should proivde useful for anyone reading this.

My main focus point here will be designing the code such that it will logically do what it is supposed to do.

You have a method in which you take a List<String[]> as argument, and you want to return the top x occurences, that is all you want, in order that means:

Put all Strings together in some structure.
Keep track of how many occurences they have.
Sort the list to have the words with the highest amount of occurences first.
Return the top x occurences.
Then decide what you want to do with it.

Issue 4 and 5 are seperated explicitely here, because that is what you ought to do. Every method should serve one purpose, and doing something and printing it is not one purpose.

Another improvement is that as input I want a structure consisting of Strings, and not your (ugly) List<String[]>, I will also deal with that. The code will be fully explained below. While we are at it, I'll also improve the error checking.

The code:

public static Stream<Map.Entry<String, Long>> getTopWords(final int topX, final Stream<String> words) {
    if (topX < 1) {
        throw new IllegalArgumentException("invalid value for topX: " + topX);
    }
    Objects.requireNonNull(words);
    Comparator<Map.Entry<String, Long>> comparator = Comparator.comparingLong(Map.Entry::getValue);
    return words.collect(Collectors.groupingBy(i -> i, Collectors.counting()))
            .entrySet().stream()
            .sorted(comparator.reversed())
            .limit(topX);
}

List<String[]> phrases = Arrays.asList(new String[]{"a", "b", "c"}, new String[]{"a", "a", "b", "d"});
List<Map.Entry<String, Long>> topEntries = getTopWords(2, phrases.stream().flatMap(Arrays::stream))
        .collect(Collectors.toList());
int counter = 1;
for (Map.Entry<String, Long> entry : topEntries) {
    System.out.println(counter + ": " + entry.getKey() + " - " + entry.getValue());
    counter++;
}

The output:

  1: a - 3
  2: b - 2

The explanation:

You have your List<String[]> phrases first, you want to simply have an object that holds your Strings. Here a Stream<String> is a suitable object, because you only need to have a view on your phrases object, there is no point in actually storing the new data. You do this by calling phrases.stream().flatMap(Arrays::stream).

This will first turn your List<String[]> in a Stream<String[]>.
Then you use a method reference that describes the lambda stringArray -> Arrays.stream(stringArray) to acquire a Stream<String>.
Then with the flatMap you add all elements of the resulting Stream<String> back into the original Stream<String>.

Then you start at the getTopWords method, which returns a Stream<Map.Entry<String, Long>>, again to offer the flexibility to do what you want with the results, they are not stored yet at the point where it gets returned.
First I added some error checking.
Then I obtain a Comparator<Map.Entry<String, Long>> that will compare what the entry is with the lowest number of occurences. This is done by using a Comparator.comparingLong on the value of the entry, which is obtained with the method reference Map.Entry::getValue.
Then I start the chain of operations on the input Stream<String>:

First group the results by their identity, which normally produces a Map<String, List<String>>.
The trick here is that I also used a downstream Collector, which counts the number of times the string occurs, hence it is called Collectors.counting().
At this point I have a Map<String, Long> denoting the word and the number of occurences. It uses a Long, because this is what Collectors.counting() returns.
Then I obtain a Set<Map.Entry<String, Long>> and convert it into a stream.
Then I call sorted() on the Stream<Map.Entry<String, Long>> with the reversed comparator. This is done here, because type interference is not strong enough to use Comparator.comparingLong(Map.Entry::getValue).reversed().
Then I limit() the stream by the top x elements.

Now we have the Stream<Map.Entry<String, Long>>, and here we decide to collect it into a List<Map.Entry<String, Long>>.
Here we continue with your old logic of having a counter attached to it.

A few points that are worth to note:

The explicit comparator.reversed() is ugly, but neccessary to not result in type casting, which is even more ugly, it is a limitation of the current type interference. It might be only an issue in IDE's and the javac compiler might actually compile it though.
The usage of Map.Entry<String, Long> is pretty bloated, but our most reasonable option, besides creating a Pair class ourselves and using Pair<String, Long>. This will hopefully be easier if Java 9 includes tuples (which logically include pairs) as more or less first-class citizens.
In the whole method getTopWords we end up storing all the entries in memory once, with the Map<String, Long>, I am pretty sure there are ways around that, but not worth the effort here, only optimize this if it becomes a real bottleneck.
I was hoping to use the Stream.forEach() method when processing the results, however this is not possible with the requirement that you want to have a counter. Again, more possibilities open up in Java 9 when we hopefully have BiStreams and tuples.

I hope this review has been helpful for you.


Filter out non-alphabetic characters from a list of words For coding practice / interview exercises, I'd like to know if there's an optimizaton I can make to the following, where I "clean" a given word to remove punctuation or other characters that are not within "a" to "z".

There are some great answers here to remove punctuation from a string, so my question today is not the best way how to do this, but instead whether there is an optimization I can make to my 3 lines of code below in the word_count_engine function? Can I do this in 1 or 2 lines or make the code more efficient so it doesn't loop over the list twice (i.e. with 2 list comprehensions)?

def clean(word):
    returnword = ""
    for letter in word.lower():
        if letter >= 'a' and letter <='z':
            # not out of bounds
            returnword += letter
    return returnword

def word_count_engine(document):

    words = document.split() # if there are     extra    spaces, split() still filters empty words out FYI
    words = [clean(word) for word in words] # a word like "$33!" will result in an empty string though
    words = [word for word in words if word] # so filter out empty strings and get the final list of clean words

document = "Practice makes perfect. you'll only get Perfect by practice. just practice! $544 test"

Since Python strings are immutable, appending one character at a time using += is inefficient.  You end up allocating a new string, copying all of the old string, then writing one character.

Instead, clean() should be written like this:

def clean(word):
    return ''.join(letter for letter in word.lower() if 'a' <= letter <= 'z')

Note that Python supports double-ended inequalities.

The name of your word_count_engine function poorly describes what it does.  In fact, the function doesn't print or return anything, so it's all dead code.  If I had to rewrite it, though, I'd say:

words = [word for word in map(clean, document.split()) if word]

Also consider replacing all of this code with a simple regular expression substitution.


function to check if word matches search word I have the following which works but was wondering if there's a better way. Better meaning more efficient and or more compact.
The function checks a trie to see if the word is in the trie.
typedef struct nodeWords
{
    char * word;
    int    index;
    struct nodeWords *left;
    struct nodeWords *right;
} nodeWords;

nodeWords ** nodeArray;

/* nodeArray is a pointer to an array of pointers. nodeArray[nodeIdx] is
   a pointer to the middle nodeWords strut where word comes from a sorted
   list. Basic divide and conquer routine. */

bool check(const char *word)
{
    int nodeIdx = hash(word);
    if (nodeIdx < 0)return false;

    nodeWords * searchNode;
    searchNode = nodeArray[nodeIdx];  //nodeArray is global
    bool whileFlag = true, returnFlag = false;

    do
    {
        if (strcmp(word,searchNode->word) == 0 )
        {
            whileFlag = false;
            returnFlag = true;
        }
        else if (strcmp(word,searchNode->word) < 0 )
        {
            if(searchNode->left == NULL)
            {
                whileFlag = false;
            }else{
                searchNode = searchNode->left;
            }
        }else{
            if(searchNode->right == NULL)
            {
                whileFlag = false;
            }else{
                searchNode = searchNode->right;
            }
        }
    }while (whileFlag);

    return returnFlag;
}

for instance, we could rewrite the function this way:
bool check(const char *word)
{
    int nodeIdx = hash(word);
    if (nodeIdx < 0)return false;

    nodeWords * searchNode;
    searchNode = nodeArray[nodeIdx];
    bool returnFlag = false;

    do
    {
        if (strcmp(word,searchNode->word) == 0 )returnFlag = true;
        else if (strcmp(word,searchNode->word) < 0 && searchNode->left  != NULL) searchNode = searchNode->left;
        else if (strcmp(word,searchNode->word) > 0 && searchNode->right != NULL) searchNode = searchNode->right;
    }while (searchNode->left != NULL || searchNode->right != NULL || strcmp(word,searchNode->word) == 0);

    return returnFlag;
}

I notice that the second function is actually not going to work as it's not quite the same as the first but we could change the while loop condition

I don't know C, my code is not tested.

Having whitespace at the end of lines of code is really annoying to work with.
I'm not a fan of if without curvy brackets. I have previously been bitten by adding another expression in the if and neglecting to add the brackets.
I would much prefer using break over while (whileFlag).
I would much prefer using return ... over returnFlag.
By changing the above we can change the do while loop to just a while (true) loop.
If you've followed along with the changes we can see that searchNode will be null when we want to break from the loop. And so we can remove the inner ifs by moving them to the while.

bool check(const char *word)
{
    int nodeIdx = hash(word);
    if (nodeIdx < 0)
    {
        return false;
    }

    nodeWords * searchNode;
    searchNode = nodeArray[nodeIdx];

    while (searchNode != NULL)
    {
        if (strcmp(word, searchNode->word) == 0 )
        {
            return true;
        }
        else if (strcmp(word, searchNode->word) < 0 )
        {
            searchNode = searchNode->left;
        }else{
            searchNode = searchNode->right;
        }
    }
    return false;
}


ThreadSafeObservableCollection of (T) The idea here is to implement a simple, threadsafe, observable collection that clients can bind to, whilst background threads can update. Changes in the contained items raise the CollectionChanged event.

Feedback appreciated:

public class ThreadSafeObservableCollection<T> : INotifyCollectionChanged
    where T: INotifyPropertyChanged
{
    private readonly Object _lock = new Object();
    private readonly Collection<T> _items = new Collection<T>();
    private ReadOnlyCollection<T> _readonlyItems;

    public ThreadSafeObservableCollection()
    {

    }

    public ThreadSafeObservableCollection(IEnumerable<T> items)
    {
        _items = new ObservableCollection<T>(items);
        foreach (T item in _items)
        {
            ((INotifyPropertyChanged) item).PropertyChanged += ItemPropertyChangedHandler;
        }
    }

    public ReadOnlyCollection<T> Items
    {
        get { return _readonlyItems ?? (_readonlyItems = new ReadOnlyCollection<T>(_items)); }
    }

    public virtual void Add(T obj)
    {
        lock(_lock)
        {
            ((INotifyPropertyChanged) obj).PropertyChanged += ItemPropertyChangedHandler;
            _items.Add(obj);
            OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, obj));
        }
    }

    public virtual void Remove(T obj)
    {
        lock (_lock)
        {
            ((INotifyPropertyChanged) obj).PropertyChanged -= ItemPropertyChangedHandler;
            _items.Remove(obj);
            OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, obj));
        }
    }

    #region INotify Members

    public event NotifyCollectionChangedEventHandler CollectionChanged;

    protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
    {
        var copy = CollectionChanged;
        if(copy != null)
        {
            CollectionChanged(this, args);
        }
    }

    private void ItemPropertyChangedHandler(object sender, PropertyChangedEventArgs e)
    {
        NotifyCollectionChangedEventArgs args = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);
        OnCollectionChanged(args);
    }

    #endregion
}

Update

In response to the comments about the ReadOnlyCollection, I have switch its implementation for a yield block that is locked (Using a ReaderWriterLock) for the length of the enumeration, like so:

    public IEnumerable<T> Items
    {
        get
        {
            _lock.AcquireReaderLock(_lockTimeout);
            try
            {
                foreach (T item in _items)
                {
                    yield return item;
                }
            }
            finally
            {
                _lock.ReleaseReaderLock();
            }
        }
    }

Any thoughts?

Your Items initialization I believe isn't threadsafe. Use Lazy<T> for thread safe lazy initialization instead.
The ReadOnlyCollection isn't thread safe.

A ReadOnlyCollection can support
multiple readers concurrently, as long
as the collection is not modified.
Even so, enumerating through a
collection is intrinsically not a
thread-safe procedure. To guarantee
thread safety during enumeration, you
can lock the collection during the
entire enumeration. To allow the
collection to be accessed by multiple
threads for reading and writing, you
must implement your own
synchronization.

Consider using a ReaderWriterLock to support a single writer, but multiple readers.

Furthermore, consider reading this reply, and the blog post mentioned there, which discusses why thread safe collections are so hard, and not necessarily useful.


Setter method for an object using a special logic I have a method that received a list of CoordinateBE : setExits(..)
I want to update the existing coordinates list with a new coordinate only if there is no coordinate in the current list that has the same latitude and longitude.
Example:
List: [Coordinate: lat:23, long:24, Coordinate: lat:34, long:24]
I am trying to add a new Coordinate: Coordinate: lat:23, long: 24 ---> this one I cannot add it because already exists in the list.
My implementation is working, but I was wondering if I can improve it, for example through lambda expressions, etc. Do you have any ideas?
// setter
  public void setExits(final List<CoordinateBE> exits) {
            if (getExits().isEmpty()) {
                this.exits = exits;
            } else {
                if (!existenceOfCoordinates(getExits(), exits)) {
                    final List<CoordinateBE> result = new ArrayList<>();
                    result.addAll(getExits());
                    result.addAll(exits);
                    this.exits = result;
                }
            }
        }

//getter
  public List<CoordinateBE> getExits() {
     return exits;
  }

// existenceOfCoordinates
 private boolean existenceOfCoordinates(final List<CoordinateBE> existingCoordinates, final List<CoordinateBE> currentCoordinates) {
    for (final CoordinateBE existingCoordinate : existingCoordinates) {
        for (final CoordinateBE currCoordinateBE : currentCoordinates) {
            if (compareCoordinates(existingCoordinate, currCoordinateBE)) {
                return false;
            }
        }
    }
    return true;
 }

// compareCoordinates
 private boolean compareCoordinates(final CoordinateBE existingCoordinate, final CoordinateBE newCoordinate) {
        return existingCoordinate.getLatitude().equals(newCoordinate.getLatitude())
                && existingCoordinate.getLongitude().equals(newCoordinate.getLongitude());
    }

Why do you recreate this.exits every time you add new items? This:
                    final List<CoordinateBE> result = new ArrayList<>();
                    result.addAll(getExits());
                    result.addAll(exits);
                    this.exits = result;

could be simplified to
                    this.exits.addAll(exits);

If coordinates are always meaningfully equal based on latitude + longitude, you may want to override equals() and hashCode() so that so you can better leverage built-in methods. With an overrided equals(), you would not need compareCoordinates, and existenceOfCoordinates could be replaced with collection methods.
For example:
private boolean existenceOfCoordinates2(List<CoordinateBE> a, List<CoordinateBE> b) {
  return b.stream().anyMatch(a::contains);
}

With the above simplifications, (I'll skip the equals() override in case you actually don't want that), it's distilled down to:
public void setExits(final List<CoordinateBE> exits) {
  if (!getExits().isEmpty()) {
    // Check that no new exits are already in the list
    for (CoordinateBE exit : getExits()) {
      if (exits.stream().anyMatch(x -> compareCoordinates(x, c))) {
        return;
      }
    }
  }

  this.exits.addAll(exits);
}

// If CoordinateBE overrides equals(), you could also do:

public void setExits(final List<CoordinateBE> exits) {
  // In this case, I'm not checking whether this.exits is empty, because
  // we're streaming the new exits as opposed to the current, and that stream
  // will just be empty if the new exits are empty.
  if (exits.stream().anyMatch(this.exits::contains)) {
    return;
  }
  this.exits.addAll(exits);
}

Finally, I am assuming that order is important here, which is why you're using a List as opposed to a Set. Even so, if you're okay with overriding equals() and if duplicates are not allowed, using a LinkedHashSet for this.exits would likely be faster beyond lists of a few dozen elements.


Check input from user Aim: Read in the userID, check if the userID has 10-digits and check if it not exists in database, when both right, the program will leave the loop.

This code works on my project, but I think its not the most elegant solution. Have someone any improvements?

InputReader.java

public class InputReader {

    private String userID;

    public void readInput() throws SQLException {

        boolean UserIDExists = false;
        int existsValue = 0;

        try {

    BufferedReader input = new BufferedReader(new InputStreamReader(System.in));

    System.out.println("UserID (10-digits):");

    while(!UserIDExists) {

        this.userID = input.readLine();

        while (this.userID.length() != 10) {

        System.out
                .println("Input must be 10 digits. Type again: :");
        this.userID = input.readLine();

        }

    CustomerInfo customer = new CustomerInfo();
    existsValue = customer.checkCustomer(this.userID);

    if (existsValue == 1) {

        UserIDExists = false;
        System.out.println("UserID exists already. Type again:");

    } else {

        UserIDExists = true;

    }
    }

CustomerInfo.java

public class CustomerInfo {

    private Connection conn = null;
    private ResultSet resultSet = null;

    public CustomerInfo () {

        try {
                conn = null;
                JDBCConnection jdbcConn = new JDBCConnection();
                conn = jdbcConn.openConnection();

        } catch (Exception e) {
                  throw e;
                }
        };

        public int checkUserID (String userID) throws SQLException {
        PreparedStatement pstmt = null;
            String sql = "SELECT * FROM TABL0001 WHERE USERID = ?";
            pstmt = conn.prepareStatement(sql);
            pstmt.setString(1,  userID);
            resultSet = pstmt.executeQuery();

            if (!resultSet.next()){
                return 0;

            }
               return 1;

        }

You are using prepared statements which is good, but why does your query return all columns ? A simple SELECT USERID FROM TABL0001 WHERE USERID = ? would be enough and if multiple users access this code, the database load would be low which is good.

The checkUserID() method is returning an int which only will be either 0 or 1 and this is screaming for a boolean which also would simplify the method. Also the name of the method is a little bit  misleading. A better name would be existUserID

Like

public boolean existUserID (String userID) throws SQLException {

    String sql = "SELECT * FROM TABL0001 WHERE USERID = ?";

    PreparedStatement pstmt = conn.prepareStatement(sql);
    pstmt.setString(1,  userID);
    resultSet = pstmt.executeQuery();

    return resultSet.next();
}

You aren't closing the database connection. Also, because you are  always creating a new CustomerInfo you really should make the former checkUserID() method static and create the connection there.

If you are using Java 7 or above you should use try with ressources to ensure that for any exception that the ressources are closed.

If you are using Java prior version 7 you should use a finally block where you the close the ressources.

The name readInput does not really say much. You should always use meaningfull names for classes, methdos and parameters. Also readInput would let me think I would get something back, but no, the method is void.

boolean UserIDExists = false;

should be named using camelCase casing.

Instead of while(!userIDExists) you should just make while(true) and if the userid does not exists just break; out of the loop.

while (true) {

    this.userID = input.readLine();

    while (this.userID.length() != 10) {

        System.out
                .println("Input must be 10 digits. Type again: :");
        this.userID = input.readLine();

    }

    // assuming existUserId() is static
    if (CustomerInfo.existUserId(this.userId)) {
        break;
    }
    System.out.println("UserID exists already. Type again:");
}

by extracting the reading from the input stream to a separate method this can be simplified some more and will reduce the responsebilities of the method.

private String readUserInput(String message) {

    System.out.println(message);

    BufferedReader input = new BufferedReader(new InputStreamReader(System.in));
    return input.readLine();
}

leading to

String message = "UserID (10-digits):";
while (true) {

    this.userID = readUserInput(message);

    while (this.userID.length() != 10) {

        this.userID = readUserInput("Input must be 10 digits. Type again: ");

    }

    // assuming existUserId() is static
    if (!CustomerInfo.existUserId(this.userId)) {
        break;
    }
    message = "UserID exists already. Type again:";
}

Based on the comment I changed the above implementation to break if the userId does not exists. But hey, the variable you formerly used UserIDExists is very very very !! misleading. One would assume (like I did) if a userId exists then the boolean variable UserIDExists would become true !


Router Class In a Lightweight MVC Boilerplate I'm trying to come up with a simple router class. I considered using a Request class, but I think that will turn something simple into something overly complex. That is why I did not consider creating one for my framework.

class Router {

    private $uri;
    private $controller;
    private $action;
    private $params;

    public function __construct($uri) {
        $this->uri = $uri;
        $this->action = 'index';
        $this->params = array();
    }

    public function map() {
        $uri = explode('/', $this->uri);
        if (empty($uri[0])) {
            $c = new Config('app');
            $this->controller = $c->default_controller;
        } else {
            $this->controller = array_shift($uri);
            if (!empty($uri[1]))
                $this->action = array_shift($uri);
            if(!empty($uri[2]))
                $this->params = $uri;
        }
    }

    public function getController() {
        return $this->controller;
    }

    public function getAction() {
        return $this->action;
    }

    public function getParams() {
        return $this->params;
    }
}

In the bootstrap, I did this to map the url:

$uri = filter_var(rtrim(isset($_GET['url']) ? $_GET['url'] : null, '/'), FILTER_SANITIZE_URL);
$router = new Router($uri);
$router->map();

What can I do to improve this?

What is a router used for?

Translate one address to another one connection two different network to eahc other depending on rules. It will never instantita a browser to display for example a HTTP request.

Your class doing to much and much lesser than it should

Your router should only deal with the input address and not instantiating a controller and other stuff. How should it work?

need a collection of rules, call RouteTable (will store Route instances)
rule: how to translate an url to a usefull address in the framwork (how to get named parameters like controller or action and so on)
the Router it self which is containing the logic

Rules, Routes

A rule should contain the following stuffs:

a pattern, say: "{controller}/{action}/{id}"

This will mean that if we have an url like book/details/5 then the framework somewhere will instantiate the book controller and call it's details method with parameter 5.

set up default values are always good practise becouse a route can work without having all parts set

For example an id is not always mandatory it can be optional or we could define a pattern like "veryimportanturl" but the framwork can handle like: controller => lottery, action => getthisweekwinningnumbers, id => 5

a good to have feature is to attach a unique name to our routes so they could be used for generating urls or form action parameters
a powerfull weapon could be if we can say to a route: "hey you have a handler, so if you are used to handle one request then you should tell how can we do that"

We can create different handlers for different routes: can have one for handling an MVC task, another one so serve special images (so we do not need to go through a complete MVC cycle can be create a thinner logic).

These rule should be mapped into our RouteTable which will be the responsible to store these and for example keep the unique route names.

Router

A router will need two things:

the current request data (rewritten url and maybe query string)
the RouteTable

The router is responsible to pick the correct one from the collection can be done different ways but here is my solution:

ordering the available routes: by the static parts (fix parameters not pattern elements ({})) and by the constraints (in a route i can set up constraint like the id onyle can be in the range of 0-5) [static length descending, constraints' numbers descending]
iterating through the ordered rules collection and trying to find out which one will be the first mach by using regular expression (the Route contains it's pattern translated into regular expression), if a match found i try to fill the pattern with data; if everything is in place i've founded otherwise continuing the search.

If the correct route has been found my routing engine returns it to the other parts of the framework but not just the pure route but mapped all the values came from the request. If nothing has found i face with an exception.

The way i see the router above does nearly nothing compared to a real router but do things what should not be done by it. Beside this it's unflexible (fixed array indexes, has a hard dependency on a Config class and a default controller (why?)).


Generic Implementation for Paging Class in c# I have created one generic Page class for store paging information with its data.

The class look like:

public class Page<T>
     where T : new()
{

    private List<T> _list = new List<T>();

    [JsonProperty(PropertyName = "total_count")]
    public long TotalCount { get; set; }

    [JsonProperty(PropertyName = "page_start")]
    public long PageStart { get; set; }

    [JsonProperty(PropertyName = "page_size")]
    public long PageSize { get; set; }

    [JsonProperty(PropertyName = "list")]
    public List<T> List
    {
        get { return _list; }
        set { _list = value; }
    }
}

For every table model of my application uses generic Page class in repository for return PageSize and its total records with its List.

My repository use like below:

public class StudentRepository
{
    private readonly CountingCloud_DevContext _dbContext;
    public StudentRepository(CountingCloud_DevContext dbContext)
    {
        _dbContext = dbContext;
    }
    public Page<StudentResponse> Get(Dictionary<string, object> filterParameters)
    {
        var resultfinal = new Page<StudentResponse>();
        int from = 0, size = 10;
        resultfinal = Paging(resultfinal, filterParameters, ref from, ref size);

        var query = _dbContext.Students.ToList();
        var result = query.Select(x => new StudentResponse
        {
            Id = x.Id,
            Name = x.Name
        });
        resultfinal.TotalCount = result.Count();
        query = result.Skip(from).Take(size);
        return resultfinal;
    }

    private static Page<StudentResponse> Paging(Page<StudentResponse> pagingResponse, Dictionary<string, object> parameters, ref int from, ref int size)
    {
        if (parameters != null)
        {
            if (parameters.ContainsKey("PageSize") && parameters.ContainsKey("PageStart"))
            {
                if (Convert.ToInt32(parameters["PageSize"]) != 0)
                {
                    size = Convert.ToInt32(parameters["PageSize"]);
                    from = (Convert.ToInt32(parameters["PageStart"]) - 1) * size;
                    pagingResponse.PageSize = size;
                    pagingResponse.PageStart = Convert.ToInt32(parameters["PageStart"]);
                }
            }
            else
            {
                pagingResponse.PageSize = size;
                pagingResponse.PageStart = from + 1;
            }
        }
        else
        {
            pagingResponse.PageSize = size;
            pagingResponse.PageStart = from + 1;
        }
        return pagingResponse;
    }
}

Currently Paging() method I have use for all table like my other Repository is for Employee then for that paging method is look like below:

private static Page<EmployeeResponse> Paging(Page<StudentResponse> pagingResponse, Dictionary<string, object> parameters, ref int from, ref int size)
{
    if (parameters != null)
    {
        if (parameters.ContainsKey("PageSize") && parameters.ContainsKey("PageStart"))
        {
            if (Convert.ToInt32(parameters["PageSize"]) != 0)
            {
                size = Convert.ToInt32(parameters["PageSize"]);
                from = (Convert.ToInt32(parameters["PageStart"]) - 1) * size;
                pagingResponse.PageSize = size;
                pagingResponse.PageStart = Convert.ToInt32(parameters["PageStart"]);
            }
        }
        else
        {
            pagingResponse.PageSize = size;
            pagingResponse.PageStart = from + 1;
        }
    }
    else
    {
      pagingResponse.PageSize = size;
        pagingResponse.PageStart = from + 1;
    }
    return pagingResponse;
}

I would like make this private method is generic so all repository use same,
please suggest me what I am implemented is correct or any better improvement for code any changes.

Some quick remarks:

Names shouldn't contain underscores etc: CountingCloud_DevContext. (The exception is the underscore at the start of a private member (e.g. _dbContext.)
If you have a Dictionary, do not use ContainsKey if you need to retrieve the value associated with that key. This is why the much more efficient TryGetValue exists.
Do not copy-paste magic strings (e.g. "PageSize") all over the place. Store them as const in a relevant class and use that.
But most importantly: what's the point of Dictionary<string, object> when all it contains is two specific key-value pairs? Why not make a PagingParameters class with PageSize and PageStart as nullable ints? Don't make things too generic, because all you end up is making things harder for yourself.
Also avoid making a generic Repository class. It likely will create more work and solve nothing.


Constructors for a class to represent a price in € or $ I'm using a method to make initialize the attributes of my objects, and this method is called from both constructor.

What it does is, if no correct currency has been passed, choose € by default.

I'm just wondering if what I've done below is correct in terms of design.

class Prix
{
    double valeur;
    string monnaie;

    public Prix(double valeur, string monnaie)
    {
        if (monnaie.Equals("€") || monnaie.Equals("$"))
        {
            this.valeur = valeur;
            this.monnaie = monnaie;
        }
        else
        {
            defaultConstr(valeur);
        }
    }

    public Prix(double valeur)
    {
        defaultConstr(valeur);
    }

    private void defaultConstr(double valeur)
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

Starting from:

class Prix
{
    double valeur;
    string monnaie;

    public Prix(double valeur, string monnaie)
    {
        if (monnaie.Equals("€") || monnaie.Equals("$"))
        {
            this.valeur = valeur;
            this.monnaie = monnaie;
        }
        else
        {
            defaultConstr(valeur);
        }
    }

    public Prix(double valeur)
    {
        defaultConstr(valeur);
    }

    private void defaultConstr(double valeur)
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

When you have multiple constructors with similar parameters, it makes sense to keep the construction logic local to one constructor. One approach is to make one constructor call straight through to a second with any extra parameters.

public Prix(double valeur)
    : this(valeur, "€")
{
}

public Prix(double valeur, string monnaie)
{
    ...
}

Alternatively, supplying a default value for optional parameters is possible:

public Prix(double valeur, string monnaie = "€")
{
    ...
}

(see this question for information about the difference between the two)
Having construction logic in one place means that defaultConstr is only being called in one place. This method call can be inlined:

public Prix(double valeur, string monnaie)
{
    if (monnaie.Equals("€") || monnaie.Equals("$"))
    {
        this.valeur = valeur;
        this.monnaie = monnaie;
    }
    else
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

We're now setting valeur in both branches, so that can be factored out:

public Prix(double valeur, string monnaie)
{
    this.valeur = valeur;

    if (monnaie.Equals("€") || monnaie.Equals("$"))
    {
        this.monnaie = monnaie;
    }
    else
    {
        this.monnaie = "€";
    }
}

There is no need to compare against € if that will be assigned anyway:

public Prix(double valeur, string monnaie)
{
    this.valeur = valeur;

    if (monnaie.Equals("$"))
    {
        this.monnaie = monnaie;
    }
    else
    {
        this.monnaie = "€";
    }
}

Next, consider what happens if we have more valid currency symbols, we could end up with code like this:

if (monnaie.Equals("$") || monnaie.Equals("£") || monnaie.Equals("¥") || monnaie.Equals("฿") || monnaie.Equals("₩"))
{
    ...
}

This is getting quite ungainly - it's best to put this into a collection that can be checked against:

class Prix
{
    static HashSet<string> monnaieValid = new HashSet<string>
    {
        "$" // , "£", "¥", "฿", "₩"
    };

    ...

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;

        if (monnaieValid.Contains(monnaie))
        {
            this.monnaie = monnaie;
        }
        else
        {
            this.monnaie = "€";
        }
    }
}

Because the collection of currency symbols is logically a set (no duplicates, unordered), a HashSet<T> is the appropriate choice for the collection. It is static because we don't need every Prix object to have its own HashSet - one shared copy is enough.
It is also advisable to avoid "magic strings" - create a constant for the default currency and use this instead of the literal string:

class Prix
{
    const string MonnaieDefault = "€";

    ...

    public Prix(double valeur)
        : this(valeur, MonnaieDefault)
    {
    }

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;

        if (monnaieValid.Contains(monnaie))
        {
            this.monnaie = monnaie;
        }
        else
        {
            this.monnaie = MonnaieDefault;
        }
    }
}

I would also make a few other small changes to help readability (although this step may be subjective):

Add access modifiers where it's currently implicit
Make fields readonly wherever possible - here no fields are modified after they are initialised, so they can all be made readonly
Use a ternary operator instead of the if statement

Finally, the class looks like this:

public class Prix
{
    private const string MonnaieDefault = "€";

    private static readonly HashSet<string> monnaieValid = new HashSet<string>
    {
        "$"
    };

    private readonly double valeur;
    private readonly string monnaie;

    public Prix(double valeur)
        : this(valeur, MonnaieDefault)
    {
    }

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;
        this.monnaie = monnaieValid.Contains(monnaie) ? monnaie : MonnaieDefault;
    }
}


Bubble sorting an int array I'd like to improve this bubble sort code

 package com.arun.sort;

 import java.util.Arrays;

 public class BubbleSort {

public static void main(String[] args) {

    int[] arr={2,5,1,8,12,3,7};
    int n=arr.length;

    for(int k=0;k<n-1;k++){
        for(int i=0;i<n-k-1;i++){
            if(arr[i]>arr[i+1]){
                int temp=arr[i];
                arr[i]=arr[i+1];
                arr[i+1]=temp;
            }
        }
    }

    System.out.println("Sorted array" +Arrays.toString(arr));

  }

   }

Step 1: format it nicely

An IDE can automatically reformat code nicely to follow the standard:

int n = arr.length;

for (int k = 0; k < n - 1; k++) {
    for (int i = 0; i < n - k - 1; i++) {
        if (arr[i] > arr[i + 1]) {
            int temp = arr[i];
            arr[i] = arr[i + 1];
            arr[i + 1] = temp;
        }
    }
}

In Eclipse it's Control-Shift-f. It adds spaces around operators. Now the code looks like the standard, and it's easier to read and code review for everyone.

Step 2: extract to a method

In the current code you have a hardcoded array, and the main logic follows right after. It's hard to test this way. What if you want to see if the implementation works with a different set of numbers? You have to rewrite the array. Better to extract the main logic into its own, independent method:

void sort(int[] arr) {
    // ...
}

Now you can test with multiple different inputs easier:

arr = new int[]{2, 5, 1, 8, 12, 3, 7};
BubbleSort.sort(arr);
System.out.println(Arrays.toString(arr));

arr = new int[]{4, 3, 2, 1, 2};
BubbleSort.sort(arr);
System.out.println(Arrays.toString(arr));

Step 3: convert print statements to proper unit tests

The problem with print statements is that every time you change something and rerun, you have to re-verify the output of each statement. Unit tests can automate the verification step, and converting is easy enough to do:

@Test
public void testMixedValues() {
    int[] arr = {2, 5, 1, 8, 12, 3, 7};
    BubbleSort.sort(arr);
    assertEquals("[1, 2, 3, 5, 7, 8, 12]", Arrays.toString(arr));
}

@Test
public void testDecreasingValues() {
    int[] arr = {4, 3, 2, 1};
    BubbleSort.sort(arr);
    assertEquals("[1, 2, 3, 4]", Arrays.toString(arr));
}

@Test
public void testDecreasingWithDups() {
    int[] arr = {4, 3, 2, 1, 2, 3, 2};
    BubbleSort.sort(arr);
    assertEquals("[1, 2, 2, 2, 3, 3, 4]", Arrays.toString(arr));
}

Now you can make changes and the test cases will flag an error if something breaks. Unless you do something really horrible, typically only a few of the test cases will break, and you don't need to reverify the others that are still working, which makes debugging a lot easier.

Minor things

Instead of n, a better name would be length to cache the length of the array.

As for the loop variables, it's more traditional to name nested counter variables as i, j, k, in this order of nesting level, instead of k, i as you did. In any case, this is really not a big deal.

int length = arr.length;

for (int i = 0; i < length - 1; i++) {
    for (int j = 0; j < length - i - 1; j++) {
        if (arr[j] > arr[j + 1]) {
            int temp = arr[j];
            arr[j] = arr[j + 1];
            arr[j + 1] = temp;
        }
    }
}


Count the number of distinct subarrays I want to determine the number of distinct subarrays that can form having at most a given number of odd elements. Two subarrays are distinct if they differ at even one position. The subarray is a contiguous position of an array. Please give some suggestions to improve the time and space complexity.
Exp1:
Input: nums = [1, 2, 3, 4], k = 1
Output: 8
Explanation: [1], [2], [3], [4], [1, 2], [2, 3], [3, 4], [2, 3, 4]

Exp2:
Input: nums = [3, 2, 3, 4], k = 1
Output: 7
Explanation: [3], [2], [4], [3, 2], [2, 3], [3, 4], [2, 3, 4]
Note we did not count [3, 2, 3] since it has more than k odd elements.

Exp3:
Input: nums = [3, 2, 3, 2], k = 1
Output: 5
Explanation: [3], [2], [3, 2], [2, 3], [2, 3, 2]
[3], [2], [3, 2] - duplicates
[3, 2, 3], [3, 2, 3, 2] - more than k odd elements

class result {

    static int numberOfSubarrays(int[] numbers, int k) {
        if(k == 0) return 0;

        boolean [] IsOdd = new boolean [numbers.length];

        for(int i = 0; i < numbers.length; i++){
            IsOdd[i] = numbers[i] %2 != 0;
        }

        HashSet<String> subs = new HashSet<String>();

        for(int i = 0; i < numbers.length; i++){
            StringBuilder sb = new StringBuilder();
            int oddCount = 0;

            for(int j = i; j < numbers.length; j++){
                if(IsOdd[j]){
                    oddCount++;
                    if(oddCount > k){
                        break;
                    }
                }
                sb.append(numbers[j] + " ");
                subs.add(sb.toString());
            }
        }
        return subs.size();
    }

}

Welcome to Stack Review, the first thing I see in your code is:
class result { ..your code }

Instead of result you should use Result as a classname because as specified in java_naming_conventions : Class names should be nouns, in mixed case with the first letter of each internal word capitalized.
The second thing I see in your code is:
HashSet<String> subs = new HashSet<String>();

Better to declare instead Set<String> subs = new HashSet<String>(); that guarantees more flexibility in case you want to use another implementation of theSet interface.
About performance, briefly you have a for loop where you instantiate a new StringBuilder object at every iteration like below:
for(int i = 0; i < numbers.length; i++){
   StringBuilder sb = new StringBuilder();
   ...other instructions
   sb.append(numbers[j] + " ");
}

There is a debated alternative to this scenario corresponding to instantiate one single StringBuilder object with a fixed capacity and resetting its length at every iteration of the loop with the StringBuilder.setLength method like below:
StringBuilder sb = new StringBuilder(Arrays.toString(numbers).length());
for(int i = 0; i < numbers.length; i++){
   ...other instructions
   sb.setLength(0);
}

For every testcase I set the capacity of the StringBuilder object to
the length of the String representation of the initial array like [1, 2, 3, 4] that is the maximal string length the StringBuilder will contain.
Update after @TorbenPutkonen comment : better to use StringBuilder.setLength(0) at the beginning of the loop to improve readibility of the code, so the reader has a clear vision of the state of the StringBuilder at the beginning and not at the end.


Comparing pixels against RGB value in NumPy Assume I created an image in NumPy:

image = imread(...)

And the image is RGB:

assert len(image.shape) == 3 and image.shape[2] == 3

I want to check (i.e. get a boolean mask) which pixels are (R=255, G=127, B=63) in a cleaner and efficient way:

mask = (img[:, :, 0] == 255) & (img[:, :, 1] == 127) & (img[:, :, 2] == 63)

This code worked for me. However - please correct me if I'm wrong - this code is creating three intermediate masks and, since I'm processing images, those masks will be pretty big.

I need help with:

Is there a way to make boolean masks occupy 1 bit per element, instead of 1 byte per element?
Is there a way to solve the same problem without creating the three intermediate boolean masks?

This code did not work:

mask = img == (255, 127, 63)

neither this:

mask = img[:, :, :] == (255, 127, 63)

since the comparison was performed element-wise, and the resulting values were performed element-wise, yielding a 3-dimension (w, h, 3) boolean mask.

Your original code can be rewritten as:

mask = np.all(img == (255, 127, 63), axis=-1)

It is a little cleaner, but not more efficient, as it still has to allocate a mask of the same size as the image.


Correct way to assign variables before a try/catch/finally using Excel = Microsoft.Office.Interop.Excel

...

Excel.Application app = null;
Excel.Workbooks books = null;
Excel.Workbook book = null;

object[,] data;

try
{
    app = new Excel.Application();
    books = app.Workbooks;
    book = books.Open("C:\...");

    Excel.Worksheet sheet = book.Worksheets[1];
    Excel.Range range = sheet.UsedRange;
    data = range.Value;
}
catch (Exception ex)
{
    throw ex;
    //To Do...
}
finally
{
    book.Close();
    app.Quit();
}

//code that the uses data object from above e.g.
...

In the code above I have to declare the objects app, books and book outside of the try block so that I have access to them in the finally block. However if I do not assign anything to them (i.e. Excel.Application app; instead of Excel.Application app = null;) then the compiler complains about the line app.Quit() saying "Use of unassigned local variable...". So is it correct to assign null to app etc in this case?

Also books doesn't get used in the finally block so is it better to declare it as I have, or should I declare it in the try block?

Two main issue I can see with your code:

app and book can potentially remain null and therefore your finally code has the potential to throw a NullReferenceException. You need to check for null first before calling methods on these objects.
The way you re-throw the exception will destroy its stacktrace. Instead of this:

catch (Exception ex)
{
    throw ex;
    //To Do...
}

you should do this:

catch (Exception ex)
{
    //To Do...
    throw;
}

As to your question where to declare books - it best practice to limit the scope of variables and declare them as close to their use point as possible.

As mentioned in the comments if you want to pass a custom error message via the exception then the best way forward it to create a custom exception which accepts the original exception as a parameter. If you derive from Exception one of the constructors accepts an inner exception parameter. So something along these lines:

public class DocumentProcessingException : Exception
{
    public DocumentProcessingException(string message, Exception innerException) :
            base(message, innerException)
    { }
}


Picking the desired one between two links I've written a script in python to scrape the link within contact us or about us from few webpages. The challenge here is to prioritize contact us over about us. For example, if any site contains both of them then my scraper should pick the link within contact us. However, if contact us is not present then only the scraper go for parsing the link within about us. My first attempt used the logic if "contact" in item.text.lower() or "about" in item.text.lower() but I could notice that in every cases while dealing with the below links the scraper picks the link within about us whereas my first priority is to get the link within contact us. I next rewrote it with the following approach (using two for loops to get the job done) and found it working.

This is what I've tried to get the links complying with the above criteria:

import requests
from urllib.parse import urljoin
from bs4 import BeautifulSoup

links = (
    "http://www.innovaprint.com.sg/",
    "https://www.richardsonproperties.com/",
    "http://www.innovaprint.com.sg/",
    "http://www.cityscape.com.sg/"
    )

def Get_Link(site):
    res = requests.get(site)
    soup = BeautifulSoup(res.text,"lxml")
    for item in soup.select("a[href]"):
        if "contact" in item.text.lower():
            abslink = urljoin(site,item['href'])
            print(abslink)
            return

    for item in soup.select("a[href]"):
        if "about" in item.text.lower():
            abslink = urljoin(site,item['href'])
            print(abslink)
            return

if __name__ == '__main__':
    for link in links:
        Get_Link(link)

The two for loops defined within the above function look awkward so I suppose there is any better idea to do the same. Thanks in advance for any betterment of this existing code.

You're right that the two for-loops are possibly overkill... though, it's not all that bad.. how big are the documents really?. The alternative is to track a default about link, and if a contact link appears, to override it. I'll explain that later, but first I should mention that the function should really return a value, not just print it inside the loops.

Having said that, consider the loop:

link = None
for item in soup.select("a[href]"):
    if "contact" in item.text.lower():
        link = urljoin(site,item['href'])
        # terminate the loop, no need to look further.
        break;
    if link is None and "about" in item.text.lower():
        link = urljoin(site,item['href'])
return link


Exporting a table with header and subheader I have an array matrix, for example A(3,3), I want to make a table with header names like, ['force','mass','acceleration'], the second row contains the units (subheader), ['N','Kg','m/s^2'], and the rest of the table contains the elements of A.

I want to export this table as a (.dat) file, how can I do that in an efficient way (I want to implement it for a large number of matrices)?

This is my code, which I believe it is not professional.

A = [1,2,3;4,5,6];

C(1,:) = {'force','mass','acceleration'};
C(2,:) = {'N','Kg','m/s^2'};

C(3,:) = num2cell(A(1,:));
C(4,:) = num2cell(A(2,:));
T = cell2table(C)

writetable(T,'tabledata.dat')

also this is a new update

A = [1,2,3;4,5,6;7,8,9];
C(1,:) = {'N','Kg','m/s^2'};
C(3:5,:) = num2cell(A(:,:));

T = cell2table(C)

T.Properties.VariableNames = {'force' 'mass' 'acceleration'};

writetable(T,'tabledata.dat','Delimiter','\t')

I'm assuming the data in variable A is calculated somehow in MATLAB, and that you have a matrix A that you want on a tab-delimited format, with headers. If that's the case, then writetable is probably your best bet (as you have figured out).

The expressions below are very hard to scale, since 100 rows of data results in 100 rows of code.

C(3,:) = num2cell(A(1,:));
C(4,:) = num2cell(A(2,:));

Simply doing data_in_cells = num2cell(A) is much more robust and clean.

The following is probably the best way to achieve what you want:

A = [1,2,3;4,5,6];

headers = {'force', 'mass', 'acceleration'};
units = {{'N'},{'Kg'},{'m/s^2'}};   % or: units = num2cell(units);

table_of_data = cell2table([units; num2cell(A)]);
table_of_data.Properties.VariableNames = headers;

writetable(table_of_data,'tabledata.dat','Delimiter','\t')

The reason why I use a double nested cell for units is that when trying to create the matrix MATLAB checks if all elements in each column are of the same type, and if so concatenates them as that type. In MATLAB, [65 66, 'C'] becomes ABC, because it converts the entire array to chars.
Since the first column contains a single character N, this column looks like:

'N'
1
4

MATLAB translates this to: char([78; 1; 4]). 1 and 4 when converted to ASCII-characters becomes: "Start of heading", and "End of transmission". These are not printable ASCII. Placing N inside a cell will result in the correct result, since MATLAB can't concatenate a cell and integers.

Note that I'm not placing headers, units and the data in a single cell as you did (C). This is because:

You don't need that cell anywhere
Avoid the additional memory overhead


jQuery cycle content of static HTML page I am making a simple jQuery code that changes the content of a static HTML page when a button (previous/next aka left/right) are pressed. The page is static, so the content that must be cycled (images setted as background, descriptions and a link) are saved in an array. Images must be shown two at a time, so the arrays must use two different counters (one for the images that goes every 2, and one for the links and descriptions that goes every element.)

var images = []; //array of images
var descriptions = []; //array of strings
var links = []; //array of urls

var i = 0;
  var j = 0;

  $(document).ready(function() {
    $(".view-before").css("background-image", 'url('+images[i]+')');
    $(".view-after").css("background-image", 'url('+images[i+1]+')');
    $(".gallery-title-h1").html(descriptions[j]);
    $("#link").attr("href", links[j]);
    animateTo(80);
  });

  $("#gallery_button_left").click(function() {
    if(i>0) {
      j=j-1;
      animateTo(80);
      $(".view-before").css("background-image", 'url('+images[i-2]+')');
      $(".view-after").css("background-image", 'url('+images[i-1]+')');
      $(".gallery-title-h1").html(descriptions[j]);
      $("#link").attr("href", links[j]);
      i=i-2;
    }
  });

  $("#gallery_button_right").click(function() {
    if(i<images.length-2) {
      i=i+2;
      j=j+1;
      animateTo(80);
      $(".view-before").css("background-image", 'url('+images[i]+')');
      $(".view-after").css("background-image", 'url('+images[i+1]+')');
      $(".gallery-title-h1").html(descriptions[j]);
      $("#link").attr("href", links[j]);
    }
  });

What I'd like to know is if performances could be improved or if it can be done in a "smarter" way than just cycling array elements.

I would try to avoid using separate arrays for values that are interrelated.
Instead create an object with the elements of each content like:

{
    images: [
        "http://www.example.com/img100.jpg",
        "http://www.example.com/img101.jpg"],
    description: "some text 1",
    link: "http://www.somewhere.com/"
}

And create an array of these objects. So you only have to handle one array and one index.

var content = [ // Array of objects
{
    images: [
        "http://www.example.com/img100.jpg",
        "http://www.example.com/img101.jpg"],
    description: "some text 1",
    link: "http://www.somewhere.com/a"
}, {
    images: ["http://www.example.com/img200.jpg",
        "http://www.example.com/img201.jpg"],
    description: "some text 2",
    link: "http://www.somewhere.com/b"
}, {
    images: ["http://www.example.com/img555.jpg",
        "http://www.example.com/img556.jpg"],
    description: "some text 3",
    link: "http://www.somewhere.com/c"
}, {
    images: ["http://www.example.com/imgA.jpg",
        "http://www.example.com/imgX.jpg"],
    description: "some text 4",
    link: "http://www.somewhere.com/d"
} ];

I would also add a "show" function to avoid repeating the same code in each case. Thus, the code that handles the content can be simplified as:

var currentIndex = 0;

$(document).ready(function () {
    /* Content initialization */
    show(content[currentIndex]);

    /* Register click handlers */
    $("#gallery_button_left").click(function () {
        if (currentIndex > 0) {
            show(content[--currentIndex]);
        }
    });

    $("#gallery_button_right").click(function () {
        if (currentIndex < content.length - 1) {
            show(content[++currentIndex]);
        }
    });

});

/* Update content function */
function show(data) {
    animateTo(80);
    $(".view-before").css("background-image", 'url(' + data.images[0] + ')');
    $(".view-after").css("background-image", 'url(' + data.images[1] + ')');
    $(".gallery-title-h1").html(data.description);
    $("#link").attr("href", data.link);
}

Demo:

var content = [ // Array of objects
{
    images: [
        "http://www.example.com/img100.jpg",
        "http://www.example.com/img101.jpg"],
    description: "some text 1",
    link: "http://www.somewhere.com/a"
}, {
    images: ["http://www.example.com/img200.jpg",
        "http://www.example.com/img201.jpg"],
    description: "some text 2",
    link: "http://www.somewhere.com/b"
}, {
    images: ["http://www.example.com/img555.jpg",
        "http://www.example.com/img556.jpg"],
    description: "some text 3",
    link: "http://www.somewhere.com/c"
}, {
    images: ["http://www.example.com/imgA.jpg",
        "http://www.example.com/imgX.jpg"],
    description: "some text 4",
    link: "http://www.somewhere.com/d"
}, ];

var currentIndex = 0;

$(document).ready(function () {
    /* Content initialization */
    show(content[currentIndex]);

    /* Register click handlers */
    $("#gallery_button_left").click(function () {
        if (currentIndex > 0) {
            show(content[--currentIndex]);
        }
    });

    $("#gallery_button_right").click(function () {
        if (currentIndex < content.length - 1) {
            show(content[++currentIndex]);
        }
    });

});

/* Update content function */
function show(data) {
 //animateTo(80);
    $(".view-before").css("background-image", 'url(' + data.images[0] + ')');
    $(".view-after").css("background-image", 'url(' + data.images[1] + ')');
    $(".gallery-title-h1").html(data.description);
    $("#link").attr("href", data.link);
}

LEFT

RIGHT

    link


Finding the largest jacobian in a finite element mesh So I'm trying to implement the following problem in a couple of different languages to get a rough idea of how different languages work & feel. I'm completely new to C, and this is the first code I've cobbled together using the C reference and Google/SO.

The problem is this:

Generate a list of n random doubles in (0, 1) and sort it
Create n "nodes" or points, so a Node is just a type with three doubles x,y,z
set x of each node to the corresponding entry in the list
generate a list of "elements", called a "mesh", where each element has two nodes
compute the Jacobian for each element (see jacobian below)
print the largest Jacobian of all elements

The following code works, I think. Since I have no experience in C, I want to know:

How can the code be made more idiomatic?
How can the code be made more performant?

#include <stdlib.h>
#include <math.h>
#include <stdio.h>

struct Node
{
    double x, y, z;
};

struct Element
{
    struct Node node1, node2;
};

struct Mesh
{
    struct Element* elements;
    int num_elements;
};

int compare_doubles(const void* a, const void* b)
{
    double arg1 = *(const double*)a;
    double arg2 = *(const double*)b;

    if (arg1 < arg2) return -1;
    if (arg1 > arg2) return 1;
    return 0;
}

double* generate_coordinates(int num_nodes)
{
    double* coordinates = malloc(num_nodes * sizeof(double));
    for (int i=0; i < num_nodes; i++)
    {
        coordinates[i] = (double) rand() / RAND_MAX;
    }

    qsort(coordinates, num_nodes, sizeof(double), compare_doubles);

    return coordinates;
}

struct Mesh generate_mesh(double* coordinates, int num_nodes, int num_elements)
{
    struct Node* nodes = malloc(num_nodes * sizeof *nodes);
    for (int i=0; i < num_nodes; i++)
    {
        nodes[i].x = coordinates[i];
        nodes[i].y = 0.0;
        nodes[i].z = 0.0;
    }

    struct Element* elements = malloc(num_elements * sizeof *elements);
    for (int i=0; i < num_nodes; i++)
    {
        elements[i].node1 = nodes[i];
        elements[i].node2 = nodes[i+1];
    }

    struct Mesh mesh;
    mesh.elements = elements;
    mesh.num_elements = num_elements;

    return mesh;
}

double jacobian(struct Element elem)
{
    double dx = elem.node2.x - elem.node1.x;
    double dy = elem.node2.y - elem.node1.y;
    double dz = elem.node2.z - elem.node1.z;
    return 0.5 * sqrt(dx*dx + dy*dy + dz*dz);
}

double find_max_jacobian(struct Mesh mesh)
{
    double max = 0.0;
    for (int i=0; i < mesh.num_elements; i++)
    {
        double jac = jacobian(mesh.elements[i]);
        if (jac > max)
            max = jac;
    }

    return max;
}

int main()
{
    int num_elements = 5000000;
    int num_nodes = num_elements + 1;

    double* coordinates = generate_coordinates(num_nodes);
    struct Mesh mesh = generate_mesh(coordinates, num_nodes, num_elements);

    printf("Max jacobian: %e\n", find_max_jacobian(mesh));
    return 0;
}

Edit: I just realised that I'm storing the nodes twice (right?). So I've made the nodes in the element just pointers, and modified the access in jacobian, so

struct Element
{
    struct Node *node1, *node2;
};
// ...
    elements[i].node1 = &nodes[i];
    elements[i].node2 = &nodes[i+1];
// ...
    double dx = elem.node2->x - elem.node1->x;

Welcome to the wonderful world of C programming where you have to do everything yourself.

The C programming language is basically a high level assembly language, there is no garbage collection and the programmer has to manage the memory themselves.

The Good

Before I start discussing what can be improved, I'll mention the good things:
 - The code is quite readable
 - No casts from malloc() to the receiving variable
 - The code follows the Single Responsibility and KISS principles, lots of nice small functions, although the function struct Mesh generate_mesh(double* coordinates, int num_nodes, int num_elements) could have 2 sub-functions to implement it, each of the for loops could be a function.
 - There is only one Magic Number, and that one is forgivable, the 5000000 used to initalize the variable num_elements

Error Checking

The allocation functions malloc(), calloc() and realloc() have been known to fail. This happens less often now than historically because computers generally have more memory than they used to, but there are still cases of limited memory such as in embedded programming.

When an allocation function fails it returns NULL. Reference through a NULL pointer causes unknown behavior, and generally the program will terminate without an explanation. It is a good habit to test the results of the allocation before using the pointer in code:

    double* coordinates = malloc(num_nodes * sizeof(double));
    if (!coordinates)
    {
        fprintf(stderr, "Allocation of coordinates failed, exiting program\n");
        exit(EXIT_FAILURE);
    }

    for (int i=0; i < num_nodes; i++)
    {
        coordinates[i] = (double) rand() / RAND_MAX;
    }

There are other ways to handle this error, you might want to learn about setjmp and longjmp. There is a discussion about setjmp and longjmp in this stackoverflow question. There are problems using exit() in some cases.

Prevent Memory Leaks

As mentioned above, C doesn't have garbage collection, the programmer is responsible for de-allocating memory through the use of free(). If this program was part of a larger more complex program there would be memory leaks because none of the allocated memory is ever freed. This could lead to other memory allocation failing.

In reference to the edit, you could free the nodes array after the content of the nodes were copied into the elements.

Prefer calloc Over malloc for Arrays

The function calloc(size_t num, size_t size) is generally preferred over malloc(size_t size) for arrays. The first parameter is the number of items to allocate, the second number is the size of the items. The parameters obviously make sense for arrays. One of the benefits of calloc() is that it zeros out each element of the array when the memory is allocated.

When allocating memory the code become more maintainable when the sizeof() function contains the variable the memory is being assigned to rather than the actual type. This allows the programmer or maintainer t change the type of the variable without having to change the code of the allocation itself. An example is :

    double* coordinates = calloc(num_nodes, sizeof(*coordinates));
    if (!coordinates)
    {
        fprintf(stderr, "Allocation of coordinates failed, exiting program\n");
        exit(EXIT_FAILURE);
    }

In the code above changing the type of the variable coordinates automatically changes the size of the items being allocated.


Returning maximum number of consecutive 1s in list of binary numbers This is what I implemented in Rust to get the maximum number of consecutive 1s in list of binary numbers.
impl Solution {
    pub fn print_values(nums: Vec<i32>) -> i32{
        let mut result = 0;
        let mut count = 0;
        // let mut end = 0;
        // NotE: Change the if so that it check
         // if 0 else have body for 1 condition

        if nums.len() < 10000 {
            for (index,num) in nums.iter().enumerate() {
                // println!("{}:{}", index, num);
                if *num == 1 && index != nums.len() {
                    count+=1;
                    // println!("count updated to {}", count);
                }else{

                    if count > result {
                        result = count;

                    }
                    count = 0;
                    // println!("count reset to {} from {}", count, result);
                }
                if index == nums.len()-1 && count > 0 {
                    // println!("Last if checked count {} result {}", count, result);
                    if count > result{
                        result = count;
                        count =0;
                    }
                }
            }
            // println!("afeter leaving for loop ending result :{} ", result);
            result

        }else{
            // what's the difference if  we put semicolon here at end or what if not
            panic!("List is too much larger!! Kindly reduce your list.")

        }
    }
}

Though I've accomplished the desired resutl, I guess as long I tested. Kindly review strictly.
How can I improve the speed and memory usage here?

I will mainly focus on style and design, instead of speed and memory usage, as I think that is where you have the most room for improvement. :)

Formatting: Always run cargo fmt (aka rustfmt) before posting your code online! It enforces standard formatting to improve readability.

Function arguments: This is a function that does not need ownership over its argument; you can count the ones in a purely read-only way. So it should take &[i32] instead of Vec<i32>. The idea is that this will be much faster and more memory-efficient if the user has a Vec<i32> but needs to retain ownership over it.

Naming: The name, print_values, is nondescriptive. A good name for this function would be count_consecutive_ones or max_consecutive_ones.
Other than this, the names for your variables look good to me.

Program logic: The implementation is a bit convoluted. A few specifics:

&& index != nums.len() is not necessary, here you know index is used to index nums so it is less than the length.

The logic in the block if index == nums.len() - 1 does not belong inside the loop, but outside. In particular, this fails if the vector is empty, but it is also just clearer to include "final" code at the end.

With this fixed, you shouldn't need index at all.

Local variables (result and count): don't initialize variables at the top of the function, but rather initialize as late as possible, when you need a particular variable.

Here's a better implementation of the main part of the body:
    let mut result = 0;
    let mut count = 0;
    for num in nums.iter() {
        if *num == 1 {
            count += 1;
            if count > result {
                result = count;
            }
        } else {
            count = 0;
        }
    }
    result

Length cap: Regarding the cap you put on the function (panic!ing if nums.len() >= 10000), I like the idea, but I think that you want to be more thoughtful about the way this is designed. Presently, if a user has a vector longer than 10000 to count, they will (1) not be able to use your function, and (2) even worse, probably run into an unexpected crash at runtime that they did not realize would occur.
The rule in Rust is to always make the possibility of errors -- especially panic! runtime errors -- explicit. I would consider the following design:

First, make the magic number 10000 configurable: const LIST_TOO_LARGE: usize = 10000.

Second, provide a version that does not fail -- e.g. max_consecutive_ones(nums: &[i32]) -> i32. If you like, you can print a warning: if nums.len() >= LIST_TOO_LARGE { eprintln!("Warning: list too large! Function may be a bit slow.") }

Third, if you like, provide a version which fails by wrapping around your non-failing version. Basically if nums.len() >= LIST_TOO_LARGE { /* fail */ } else { max_consecutive_ones(nums) }
I would return Result<i32, String> (expose the error explicitly) in this version instead of using panic!().

Another design you could consider: you could define
max_consecutive_ones_throttled(nums, throttle: Option<usize>), where if throttle is specified then you only check the first throttle values. That seems to go along with the idea of providing a cap for efficiency, but now you have the advantage that (1) users of your code can choose a throttle specific to their use case; (2) you don't just fail if the list is too long, but you still at least check the first throttle values.

Answering your question about semicolons after panic: whether you have a semicolon or not doesn't make any difference. It is up to you what style you prefer.