Moving files from a directory to multiple with names same as file I have a folder containing thousands of files and directories. I
  would like to move all the files that are not contained in a sub
  directory into a directory and name it the file name, while leaving
  the files already inside of directories alone, ie:

  create 'newdir' ;   move 123.mp4 ; into 'newdir' ; change name 'newdir' to
  '123' ; repeat .

In response to the mentioned question, I posted an answer with the following script.

#%%
import os

# %%
source = "/Users/me/Desktop/folder" # notice the missing last "/"
filenames = next(os.walk(source))[2]
filenames = [ x for x in filenames if not ".DS_Store" in x]
#%%
dirpaths = [os.path.splitext(file)[0] for file in filenames]

# %%
def making(directory,source=source):
   directory = os.path.join(source,directory+"/")
   os.makedirs(directory)

a = [making(directory) for directory in dirpaths]     #list comprehension could be faster than loops.

# %%
def movingfiles(onefile, source=source):
   os.rename(source + "/" + onefile, source + "/" + os.path.splitext(onefile)[0] + "/"  + onefile )

b = [movingfiles(onefile) for onefile in filenames]       #useless list again.

I want to know whether list comprehensions are better the way I used them. Also, I know that I can avoid the directory list and making a lot of empty directories at once by putting them in one function (and make "the" directory on the spot), but can anything else be improved? For e.g., I had to put a note to avoid putting / in source. Also, is this way of using lists to perform a function actually helpful?

Like you noticed, no need to use multiple loops when you can do everything in one function and loop only once.

Aside from that, there are many small things that can be improved with this script:

This script doesn't work if the script is not in the same directory as the source directory. Correct if me I am wrong, but this is what I encountered when I tested the code. To allow for this flexibility (of using this script at any arbritary location), it is necessary to use absolute paths.
We should probably check if the directory we're about to create already exists. It may be possible that two images may have the same basename but different extensions. For this case, using os.path.exists will save us down the road and is simple to implement.
I noticed you excluded .DS_Store from filenames. I took it a step further and excluded all hidden files and files that started with __ (like Python build directories/files).
When using a loop comprehension for the sake of the for loop and not for the results, there's no need to store the output. For example, when using a = [making(directory) for directory in dirpaths], it is more useful to only write [making(directory) for directory in dirpaths] on that line. Storing it as a result implies that the output is being used somewhere later in the script, when in this case, it is not useful in the final version.
It is better to use shutil.move for the movingfiles function. Under the hood, Python will decide whether to use os.rename or if a copy-remove operation is neccesary (see shutil.move for more information).
Using os.makedirs is overkill for this example here. os.makedirs is meant for recursively creating directories, not for single-level directory creation. The similiarily named os.mkdir is more appropriate here and probably will be more performant.
It is not necessary to check if the source directory string contains a '/'. On the documentation, os.path.join verifies that the result will have "exactly one directory seperator". Therefore, we don't have worry about extra logic.
Normally, I wouldn't bother with optimization when the context of the script is for running a few files. In the case of the thousands of objects, however, it is crucial to reduce how many loops we run. In the code below, you see implicit use of generators. Specifically, we will lazily evaluate our list of filenames upto the point we process each filename. This means we only run through our list of filenames once(!)
On the topic of performance, when we have a custom function defined (read: not lambda), it almost always better to use map when we don't require storing the output. On Python2, the advantage was neglible, but on Python3, the difference is noticable. So we will use map below. (map is very much faster when using builtin Python functions that call on C code. Why? Because map also reaches down to C, meaning you get a barebones C loop for those operations. Looping in Python is unintuitively bloated compared to C.)
The use os.path.splitext is correct for this script. However, there is a slight performance cost to using this function, and so I opted for the leaner .split('.', 1)[0] which is faster. As long as the filenames don't contain '.' other than at the end as an extension, we'll be fine.

#!/usr/bin/env python
import os
import shutil
import argparse
import functools

def mkdir_move_file(file, source):
    directory = file.split('.', 1)[0]
    if not os.path.exists(directory):
        os.mkdir(directory)
    shutil.move(file, directory)

def mkdir_move_all_files(source):
    if not os.path.exists(args.source):
        raise Exception('Source folder cannot be found or does not exist!')
    filenames = (os.path.join(source, x) for x in next(os.walk(source))[2]
                 if not x.startswith('.') and not x.startswith('__'))

    # basically the same as [mkdir_move_file(file, source) for file in filenames]
    map(functools.partial(mkdir_move_file, source=source), filenames)

if __name__ == '__main__':
    parser = argparse.ArgumentParser(description='Make folders for loose files in folder.')
    parser.add_argument('source', action='store', type=str, help='The source directory to process.')

    args = parser.parse_args()

    mkdir_move_all_files(args.source)

Some further notes, beyond the initial scope of the question:

Python makes it simple to create scripts-- the key is using the argparse library. This library lets us parse arguments given through the command line, which can then be passed along the functions internally. Simply add parsing to your script and you can use the executable script from the command line!
If our mkdir_move_all_files function was more complicated/intensive, we could use asynchronous programming to speed up our overall program. That way we can fire off multiple evaluations without waiting for the result to complete. However, I don't see it being hugely beneficial for this case.

If you have any questions let me know.


Binary search implementations comparison I implemented a binary search algorithm. And did it using recursive and non-recurcive approaches. Then I compared the performance of both implementations with java out-of-the-box solution. And what I got is my implementations execute for pretty the same time as Arrays.binarySearch(), which seems to be perfectly valid result as far as the execution lasts only for log(n) time. Despite the theoretical conclusions, I am still in doubts. Are the implementations good enough and how can I heavy load the test case to notice any, let it be tiny, distinction?

Any feedback and suggestions are highly appreciated.

import java.time.*;
import java.time.temporal.*;
import java.util.*;
import java.util.stream.*;

//The program has two modes: recursive and ordinary.
//One can choose recursive mode using command line argument R.
//Or, skipping an argument, run the program in ordinary mode.
public class BinarySearch{
    private int[] array;
    private BinarySearch(int[] array){
        //We have to use sorted array to perform binary search
        Arrays.sort(array);
        this.array = array;
    }
    //Recursive search
    private int searchUtilRecursive(int[] array, int start, int finish, int item){
        if((finish - start) == 1 && array[start] == item) return start;
        else if((finish - start) == 1 && array[start] != item) return -1;
        int middle = (finish - start)/2;
        if(array[start + middle] <= item) return searchUtilRecursive(array, start + middle, finish, item);
        else return searchUtilRecursive(array, start, start + middle, item);
    }
    //Ordinary search
    private int searchUtil(int[] array, int start, int finish, int item){
        start = (finish - start)/2;
        while((finish - start) >= 1){
            if(array[start] < item) start += (finish - start)/2;
            else finish = start;
            if((finish - start) == 1 && array[start] == item) return start;
            else if((finish - start) == 1 && array[start] == item) return -1;
        }
        return -1;
    }
    private int search(String mode, int item){
        switch(mode){
        case "R": System.out.println("Start test in Recursive mode");
            return searchUtilRecursive(array, 0, array.length, item);
        default: System.out.println("Start test in Ordinary mode");
            return searchUtil(array, 0, array.length, item);
        }
    }
    public static void main(String[] args){
        //The idea of the test is to examine log n time complexity.
        //We need to use large enough array to perform a search.
        //It helps us detect tiny distinctions between Java SE
        //implementation and the custom search implementation.
        int numberOfRandoms = 10000000;
        int randomNumberToPick = new Random().nextInt(numberOfRandoms);
        IntStream intStream = new Random().ints();
        int[] intArray = intStream.limit(numberOfRandoms).toArray();
        BinarySearch binarySearch = new BinarySearch(intArray);

        Instant beforeInstantMyImpl = Instant.now();
        int resultMyImpl = binarySearch.search(args.length > 0 ? args[0] : "", intArray[randomNumberToPick]);
        Instant afterInstantMyImpl = Instant.now();

        Instant beforeInstantJavaImpl = Instant.now();
        int resultJavaImpl = Arrays.binarySearch(intArray, intArray[randomNumberToPick]);
        Instant afterInstantJavaImpl = Instant.now();

        assert resultMyImpl == resultJavaImpl: "Search result doesn't equal to Java SE implementation of search";
        assert beforeInstantMyImpl.until(afterInstantMyImpl, ChronoUnit.NANOS) == beforeInstantJavaImpl.until(afterInstantJavaImpl, ChronoUnit.NANOS): "Search is not as efficient as Java SE implementation";
        System.out.println("Test is passed");
    }
}

Enhancing your benchmark

You might be able to enhance your benchmarks a little bit. Its generally worthwhile to run a test multiple times, as there are often other factors that will affect performance on your machine. This can include other programs running on your computer, available RAM/CPU to your process, etc. I find that I typically want to run something as many times as is reasonable within a given amount of time.

Once you've done that you'll have to find a reasonable metric to use; I typically go with the mean and also report on the standard error of the mean.

It's also often useful/interesting to compare performance at multiple problem sizes, but you might not be interested in that.

You'll also want to make sure that your benchmark actually takes long enough to run; I find that you at least want to be able to notice it pause to run. If that isn't the case here (I have no idea, I haven't run it) then add a few zeros at the end of numberOfRandoms. Eyeballing it, it looks like it should be fine as-is though.

One other thing to consider is that there is the possibility that you've loaded some of the values from the array into the cache, which can artificially enhance the performance of your second run. My general preferred strategy will look something like this:

repeat n times
    generate massive random array
    run version a
    do a ton of operations on a different massive array to make sure the cache is clean
    run version b
    kill cache again

My Java is rusty, but IIRC some of the newer versions allow higher order functions of some kind, in which case I'd write a function that performs the timing and cache clearing for you and takes a function as a parameter. If your version supports doing that I'd consider adding that as well.

Miscellaneous comments

You calculate the difference between finish and start multiple times in both searchUtilRecursive and searchUtil; just calculate it once at the top and then use that value.

Using a string to represent your mode in search is icky - make an enum.

Why are you sorting the array inside your BinarySearch constructor? Just pass it a sorted array. It's unexpected to me that if I call BinarySearch on an array that it will first sort it.

On that note, BinarySearch is weird to have as a class; I'd rather have your search functions be static functions (when do I ever want an instance of a BinarySearch?).

I really dislike that you have statements on the same lines as your if and else - these are small enough functions that it doesn't hurt you to put them on multiple lines, and its way more readable that way.


Is there anyway to use less if statements in my function? I have made a random password generator using a class called password and a method called generate.
My program works as it should. It generates a random password determined by the users preferences for length, upper or lowercase, numbers and special characters.
I was just wondering if there was a way to refactor the numerous if statements I have used to determine what sort of password the program would generate.
Any other suggestions for improvements I could make would also be helpful. Thanks a ton :D
Code:
import random
import string

class password:
    def __init__(self, length, string_method, numbers=True, special_chars=False):
        self.length = length
        self.string_method = string_method
        self.numbers = numbers
        self.special_chars = special_chars

    def generate(self, iterations):

        # Checking what type of string method the user has asked for

        if self.string_method == 'upper':
            stringMethod = string.ascii_uppercase
        elif self.string_method == 'lower':
            stringMethod = string.ascii_lowercase
        elif self.string_method == 'both':
            stringMethod = string.ascii_letters

        # Checking if the user has asked for numbers or not

        if self.numbers == True:
            stringNumbers = string.digits
        elif self.numbers == False:
            stringNumbers = ''

        # Checking if the user has asked for special characters or not

        if self.special_chars == True:
            stringSpecial = string.punctuation
        elif self.special_chars == False:
            stringSpecial = ''

        characters = stringMethod + stringNumbers + stringSpecial

        # Generating the password

        for p in range(iterations):
            output_password = ''
            for c in range(self.length):
                output_password += random.choice(characters)
            print(output_password)

# Test

password1 = password(20, 'lower', True, False) # password length = 20, string method is lowercase, numbers are true and special characters are false
password1.generate(3) # generate the random password 3 times```

You can use the ternary operation to reduce the vertical space, i.e. x = a if condition else b. This sets x to a if condition is true and to b otherwise.
You can also use a dictionary to map a string to some object, like
the_map = {'a': 4, 'b': 7, 'c': 43}`
x = the_map['c']

This will set x to 43.
Then you can evaluate everything in the initializer instead. So it's not less if checks per se, but it's a bit cleaner, and you can call generate multiple times without needing to do the checks.
import random
import string

class Password:
    def __init__(self, length, string_method, numbers=True, special_chars=False):
        self.length = length
        self.string_method = {
            'upper': string.ascii_uppercase,
            'lower': string.ascii_lowercase,
            'both':  string.ascii_letters
        }[string_method]
        self.numbers = string.digits if numbers else ''
        self.special_chars = string.punctuation if special_chars else ''

    def generate(self, iterations):
        characters = self.string_method + self.numbers + self.special_chars

        # Generating the password
        for p in range(iterations):
            output_password = ''
            for c in range(self.length):
                output_password += random.choice(characters)
            print(output_password)

# Test

password = Password(20, 'lower', True, False)
password.generate(3) # generate the random password 3 times

If you actually want less checks, then just pass in the characters directly. Passing in the length and which characters you want to use into password generator seems perfectly legitimate and straight-forward. This also allows you to do more complex things like generating a password with just vowels, without even numbers, or maybe if you want to exclude certain punctuation.
import random
import string

class Password:
    def __init__(self, length, characters):
        self.length = length
        self.characters = characters

    def generate(self, iterations):
        # Generating the password
        for p in range(iterations):
            output_password = ''
            for c in range(self.length):
                output_password += random.choice(self.characters)
            print(output_password)

# Test

password = Password(20, string.ascii_lowercase + string.digits)
password.generate(3) # generate the random password 3 times


Palindrome algorithm This code could be better?

This is the algorithm:

Compare the 1st character to the last character
Compare the 2nd character to the second last character and so on
Stop when the middle of the string is reached
Just words

namespace Palindrome
{
    class Class1
    {
        static void Main(string[] args)
        {
            while (true)
            {
                Console.WriteLine("Digit a word:");
                string word = Console.ReadLine();

                if (word == "exit")
                    break;

                VerifyPalindrome(word);
            }
        }

        private static void VerifyPalindrome(string word)
        {
            bool compare = true;
            int i = 0;
            while (i < word.Length- 1)
            {
                if (compare)
                {
                    for (int j = word.Length - 1; j >= 0; j--)
                    {
                        if (word[i] == word[j])
                            compare = true;
                        else
                        {
                            compare = false;
                            break;
                        }
                        i++;
                    }
                }
                else
                    break;
            }

            if (compare)
                Console.WriteLine("This is a Palindrome word");
            else
                Console.WriteLine("This is NOT a Palindrome word");
        }
    }
}

As already stated you don't need to set it to true multiple times.
There is cleaner syntax for this.

static bool IsPalidrone(string word)
{
    for (int i = 0, j = word.Length - 1; i < j; i++, j--)
    {
        if (word[i] != word[j])
        {
            return false;
        }
    }
    return true;
}


Fetching table using mysqli, closing statement before displaying results I am confused if I can do like this:

$param = "%{$id}%";
$stmt = $conn->prepare("SELECT name, comp, date FROM stats WHERE date = ? AND name LIKE ? ORDER BY date DESC LIMIT 20");
$stmt->bind_param("ss", $param, $_POST['date']);
$stmt->execute();
$result = $stmt->get_result();
$count_rows = $result->num_rows;
$stmt->close(); // CAN I CLOSE IT HERE

while($row = $result->fetch_assoc()){
    echo '
    <tr>
        <td>'.$row['name'].'
        <td>'.$row['comp'].'
        <td>'.$row['date'].'
    </tr>';
}

Question is can i close $stmt->close(); where i do it now or should i close it after while loop?

I tested and it does work but is it wrong?

As soon as you are finished using your $stmt object, it is practical to close it.

However, I find that I am never calling $stmt->close(); in any of my projects because php is going to trash them as soon as my script is done anyhow.

Technically, you could have called it one line earlier.

Some leisure reading:

$stmt->close() vs $stmt->free_result()
When to close Prepared Statement
PHP - close prepared stmt
When is the right time to close a prepared statement?


HackerRank Ransom Note, solved using Data.Map as a hash table I try to solve this hackerrank using Haskell but not sure if I use Data.Map corretly.

Problem in brief:

given 2 strings, magazine and note
check if you can make note from words in magazine

Here is my code, which passes all the tests.

import Control.Applicative
import qualified Data.Map as M

readInts =  map read . words <$> getLine :: IO [Int]

main = do
  [a,b] <- readInts
  ms <- words <$> getLine
  ls <- words <$> getLine

  -- create magazine words table
  let mss = zip ms (repeat 1)
  let mms = M.fromListWith (+) mss

  -- create note words table
  let lss = zip ls (repeat 1)
  let lms = M.fromListWith (+) lss

  -- check for intersections, decrease wordcount for every intersection
  let dm = M.intersectionWith (-) mms lms

  -- see if any words need more than it avaliablity
  let a = M.filter (<0) dm

  -- print answer
  putStrLn $ if a == M.empty then "Yes" else "No"

If I would do this problem in python I would use a dictionary of int with string key

for example;

magazine["word"] = 3
note["word"] = 1

would give a Yes answer since there is enougth word to use

This will be a list of "don't"s, since your code is very readable, but contains some code smells. It doesn't actually solve the puzzle, but more on that later.

Data.Map.Map is not a hashtable

I just want to get something out of the way first: Data.Map isn't a hash table. A hash table needs a function \$\text{hash}: A \to I\$, where \$I\$ is some finite, bounded and integral set with \$|A|\gg|I|\$. An efficient hash map/table uses buckets and provides a best case \$\mathcal O(1)\$ access if implemented correctly. Data.Map only provides \$\mathcal O(\log k)\$ access, since it's implemented as a tree. That's why you need the Ord constraint on almost all functions in Data.Map.

However, it's still a nice data structure for this problem.

Python's dictionaries on the other hand are based on hashable functions.

Don't give names to things you won't use

  [a,b] <- readInts

You don't actually use a or b. You don't actually use that line at all. That line is meant for languages like C, where you need to manage memory yourself, e.g.

int ok = scanf("%d %d",&a,&b);
ptr_t words = malloc(...);
ptr_t words = malloc(...);

You get it. However, since you never use them, just ignore that line, e.g.

main = do
  _ <- getLine -- ignore first line
  ms <- ...

Don't repeat yourself

The first thing one notices immediately is duplicate code:

  -- create magazine words table
  let mss = zip ms (repeat 1)
  let mms = M.fromListWith (+) mss

  -- create note words table
  let lss = zip ls (repeat 1)
  let lms = M.fromListWith (+) lss

That should be a function, e.g.

count :: Ord a => [a] -> Map a Int
count xs = M.fromListWith (+) (zip xs (repeat 1))

An intersection isn't what you want

Let's think about the following magazine and words:

> let ms = words "this stranger was giving a code review. the next thing will blow your mind"
> let ws = words "we have your daughter"

Lets use count on bothmsandwsand thenintersectionWith`:

> intersectionWith (-) (count ms) (count ws)
fromList [("your",0)]

Huh. That's strange. Could we actually form that ransom note? No. We're missing "we", "have", and "daughter". So what can we conclude?

intersectionWith isn't correct, since there might be elements in the ransom note that aren't in the magazine.
Hackerrank is missing edge cases.

What should we do instead? Well, we can flip the sign on the words map and then use the union:

subtractMap :: (Ord k, Num a) => Map k a -> Map k a -> Map k a
subtractMap a b = M.unionWith (+) a (fmap negate b)

Why is it important to negate, and not simply unionWith (-)? Because the operation will only take effect if there are elements with the same key in both maps.

Put it all together

So, you've used Data.Map correctly, except for the intersectionWith, which didn't come up because the test cases were borked. Otherwise, everything was fine, except for the things mentioned above.

Here's the code that implements all the suggestions above. Note that type signatures on top-level functions are very recommended:

import Control.Applicative
import           Data.Map (Map)
import qualified Data.Map as M

count :: Ord a => [a] -> Map a Int
count xs = M.fromListWith (+) (zip xs (repeat 1))

subtractMap :: (Ord k, Num a) => Map k a -> Map k a -> Map k a
subtractMap a b = unionWith (+) a (fmap negate b)

main :: IO ()
main = do
  _ <- getLine -- ignore first line
  mss <- (count . words) <$> getLine
  lss <- (count . words) <$> getLine

  -- subtract occurrences in the words from the magazine
  let dm = subtractMap mms lss

  -- see if any words need more than it avaliablity
  let a = M.filter (<0) dm

  -- print answer
  putStrLn $ if M.null a then "Yes" else "No"

You could also put words into count, if you want to get the duplication even more down, or you could write

solve :: String -> String -> Bool
solve ms ws = M.null (M.filter (<0) (subtractMap (count ms) (count ws)))

main = do
   getLine
   a <- solve <$> getLine <*> getLine

   putStrLn $ if a then "Yes" else "No"

But at that point the code get's rather terse and unreadable, and that's up to personal preference.


Android "ServiceListener" class for listening web services I'm new in Android, Java and the whole object-oriented programming, I'm trying to develop an app which needs to listen on many web services (PHP scripts written by me returning a standardized JSON string) and I thought I could design a class which takes this commitment: ServiceListener.

import org.json.JSONException;
import org.json.JSONObject;

import java.io.IOException;

import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

public abstract class ServiceListener implements runnable {
    public static final String SERVICE_AIRPORTS = "http://example.com/app/a/services/json/airports.php";
    // ...As many constants as web services
    private static final int STATUS_CODE_FORMAT_ERROR = 5;
    private static final int STATUS_CODE_EXECUTION_FAILED = 3;
    private static final int INTERNAL_ERROR = -1;
    private static final int GENERIC_ERROR = 0;

    private JSONObject jsonResult, dataToSend;
    private String serviceUrl;

    protected ServiceListener(String serviceUrl, JSONObject dataToSend) {
        this.serviceUrl = serviceUrl;
        this.dataToSend = dataToSend;

        new Thread(this).start();
    }

    protected JSONObject listenToService() throws JSONException {
        String jsonString;
        OkHttpClient client = new OkHttpClient();
        Request request = new Request.Builder().url(serviceUrl + "?data=" + dataToSend.toString(0)).build();
        try (Response response = client.newCall(request).execute()) {
            jsonString = response.body().string();
            jsonResult = new JSONObject(jsonString);

            if(jsonResult.has("Status")) {
                if(jsonResult.getString("Status").equals("Success")) {
                    return jsonResult;
                }
                else if(jsonResult.getString("Status").equals("Failed")) {
                    jsonResult.put("StatusCode", STATUS_CODE_EXECUTION_FAILED);
                    jsonResult.put("StatusMessage", "Service failed execution");
                    return jsonResult;
                }
            }
            else {
                jsonResult.put("StatusCode", STATUS_CODE_FORMAT_ERROR);
                jsonResult.put("StatusMessage", "Service invalid format");
            }
        }
        catch (IOException e) {
            e.printStackTrace();
            jsonResult.put("StatusCode", INTERNAL_ERROR);
            return jsonResult;
        }
        jsonResult.put("Error", GENERIC_ERROR);
        return  jsonResult;
    }
}

Then you can get the response wherever you want using this:

try {
    JSONObject dataToSend = new JSONObject(/*data to send with GET e.g: UserToken, Query and so on...will be interpreted on the web service*/);
    new ServiceListener(ServiceListener.SERVICE_DATE_SELECTOR, dataToSend) {
        @Override public void run() {
            try {
                JSONObject jo = new JSONObject().put("Content", listenToService());
                // Do whatever you want with the JSONObject (jo)
                Log.d("joContent", jo.getJSONObject("Content").toString(0));
            } catch(JSONException e) {
                e.printStackTrace();
            }
        }
    };
} catch(JSONException e) {
    e.printStackTrace();
}

Is it a good and fast way to request for (my own) web services? Eager to receive criticism

You may consider abstracting the idea of an HttpService that provides capabilities to make remote endpoint invocations via http.

interface HtppService {
   <T> T get(URI endpoint, Class<T> responseType);
   <T> T put(URI endpoint, Object body, Class<T> responseType);
   <T> T post(URI endpoint, Object body, Class<T> responseType);
   ///...
}

The advantages are:

You encapsulate the reusable logic of making http remote calls, since many of your services will want to do this over and over.
Your remote services no longer need to worry about how to make remote calls, they will be provided with your own HttpService implementation for that matter. Then you can focus on the important business data.
Your implementations can vary, and today you can have one using OkHttp but tomorrow you might just as well use something like, e.g. Spring RestTemplate.
Also you totally encapsulate and control your dependencies of third-party libraries. In this case, your dependency with OkHttp won't be all over the place, but only in the implementation of this interface.

Wit this interface defined you could first work in the implementation of its methods, expecting that all your remote services would use them.

class OkHttpService implements HttpService {

   private final HttpClient httpClient;

   public(HttpClient httClient) {
      this.httpClient = client;
   }

   public <T> get(URI uri, Class<T> respType) {
     //TODO: implement generic get call
   }

  <T> T put(URI endpoint, Object body, Class<T> responseType) {
     //TODO: implement generic put call
   }

   //etc, etc

}

Then you could finally build your remote service. I would start by defining its interface:

interface AirportService {
  Airport[] getAirportsByCity(String city);
}

Then you can provide an implementation of this service that will make remote calls through your HttpService:

class DefaultAirportService implements AirportService {

   private final String host;
   private final HttpService httpService;

   public DefaultAirportService(String host, HttpService httpService) {
      this.host = host;
      this.httpService = httpService;
   }

   public Airport[] getAirportsByCity(String city) {
      URI endpoint = URI.create(String.format("%s/airports?city=%s", host, city));
      try {
          return httpService.get(endpoint, Airport[].class);
      }
      catch(Exception e) {
        throw AirportServiceException("Failed to get airports for " + city, e);
      }
   }
}

There are few advantages here:

First, by passing an HttpService instance as an argument to the constructor you can easily mock that service and test your AirportService implementation in isolation.
Also, the fact that we accept the host name as one of the arguments will help you configure different instances of your service for different environments. For example, in integrations servers you may want the endpoint to point to a test server.
Finally, you can concentrate on your business logic here, and avoid having to worry about Http calls and how to deal with them.

Once you have reached that point, if you want to encapsulate your service in a runnable object, that is going to be quite simple to do:

AirportService airportService = new DefaultAirpotService("http://localhost:8080", myHttpService);

Runnable runService = new Runnable() {
   public void run() {
      aiportService.getAirportsByCity("Miami");
   }
}
new Thread(runService).start();


Finding shortest path in matrix Given an N x N matrix of positive integers, I need to find the shortest path from the first cell of matrix to the last cell of matrix. I can move exactly x steps from any cell, where x is the value of the cell. The possible movements are right or down, i.e from any cell M[i][j] I can move to locations M[i + M[i][j]][j] or M[i][j +M[i][j]].

For example:

int[][] matrix =
    {
        {1, 2, 1},
        {1, 1, 1},
        {1, 1, 1}
    };

Returning it as a list of strings, the output would be: [(0, 0), (0, 1), (2, 1), (2, 2)]

I have implemented a working solution, but I am wondering if I can improve anything in terms of time complexity?

Here is the code:

class Node {

    // queue node used in BFS
    // (x, y) represents coordinates of a cell in matrix
    int x, y;

    // maintain a parent node for printing final path
    Node parent;

    Node(int x, int y, Node parent) {
        this.x = x;
        this.y = y;
        this.parent = parent;
    }

    @Override
    public String toString() {
        return "(" + x + ", " + y + ')';
    }
}

class Solution {

    LinkedList<String> shortestPathList = new LinkedList<>();

    public List<String> getShortestPath(int[][] matrix) {

        if (matrix == null) {
            return Collections.emptyList();
        }
        Node node = findPath(matrix, 0, 0);
        AddToPath(node);
        return shortestPathList;
    }

    // possible movements
    private static int[] row = {0, 1};
    private static int[] col = {1, 0};

    // The function returns false if is not a valid position
    private static boolean isValid(int x, int y, int matrixLength) {
        return (x >= 0 && x < matrixLength) && (y >= 0 && y < matrixLength);
    }

    // Find shortest route in the matrix from source cell (x, y) to
    // destination cell (N - 1, N - 1)
    public static Node findPath(int matrix[][], int x, int y) {

        int matrixLength = matrix.length;
        // create a queue and enqueue first node
        Queue<Node> q = new ArrayDeque<>();
        Node src = new Node(x, y, null);
        q.add(src);

        // set to check if matrix cell is visited before or not
        Set<String> visited = new HashSet<>();

        String key = src.x + "," + src.y;
        visited.add(key);

        // run till queue is not empty
        while (!q.isEmpty()) {
            // pop front node from queue and process it
            Node curr = q.poll();
            int i = curr.x, j = curr.y;

            // return if destination is found
            if (i == matrixLength - 1 && j == matrixLength - 1) {
                return curr;
            }

            // value of current cell
            int n = matrix[i][j];

            // check both 2 possible movements from current cell
            // and recur for each valid movement
            for (int k = 0; k < 2; k++) {
                // get next position coordinates using value of current cell
                x = i + row[k] * n;
                y = j + col[k] * n;

                // check if it is possible to go to next position
                // from current position
                if (isValid(x, y, matrixLength)) {
                    // construct next cell node
                    Node next = new Node(x, y, curr);

                     key = next.x + "," + next.y;

                     // if it not visited yet
                     if (!visited.contains(key)) {
                         // push it into the queue and mark it as visited
                         q.add(next);
                         visited.add(key);
                     }
                 }
             }
         }

         // return null if path is not possible
         return null;
     }

     //function to get path from start to end
     private void AddToPath(Node node) {
         if (node == null) {
             return;
         }

         shortestPathList.addFirst(node.toString());
         printPath(node.parent);
     }
}

Access Modifiers & Variables

Firstly, I would recommend you only declare one variable per line. This improves readability and also encourages & facilitates commenting the declarations.  It can however have the disadvantage of having an ugly wall of declarations at the start of your classes, though ordering your variables properly can help alleviate this problem.

I personnally like to order class & instance variables like so :

- first, `static final` variables, with a name written in capitals snake_case, e.g. THIS_IS_A_CONSTANT

- then, other `static` variables

- `final` instance variables

- within those, I like placing objects & composite types first, followed by variables of primitive types (`int`, `boolean`, `char` and so on)

- finally, I like having all `public` variables first, then `protected` and finally `private` variables, although I like to put constants at the top whether they are `public` or `private`

Of course, this is only one way of doing it, and some people may think mine is strange or doesn't suit their personal style. I'd recommend you find or come up with one that you like and stick to it in the future.

In practice, it would look something like this :

public class MyClass {
    private static final int SOME_CONSTANT = 25;
    public static final int SOME_OTHER_CONSTANT = 25;
    public static int aaa;
    public final int A;
    public List<Integer> integers;
    public Object object;
    public int x;

    ...

    public void someMethod() { ...  }

    private void aPrivateMethod() { ... }

    public static void someOtherMethod() { ... }

    ...

    public int getX() {
        return x;
    }
}

Obviously, the names given to the variables in my example are terrible and I only chose them because I couldn't be bothered to come up with a unique name for each one.

You should also keep all you variable declarations together, and not mix them with method declarations. public variables should be avoided as they can be modified by anyone or anything; instead, you can declare them as private and add getters and/or setters to access them. This can also allow you to add logic inside the getter or setter methods, to check or validate the value provided for example.

For methods, I again like placing all public methods first, followed by protected and then private ones; I generally put static methods at the bottom if there are any. And finally, I like to keep all getters & setters together, somewhere I can easily forget about them after they've been implemented.

Naming and Comments

Your code has a lot of comments, although most of them are superfluous and sometimes even wrong (e.g. run till queue is not empty followed by while (!q.isEmpty())). I personnally don't like comments very much and try to avoid them as much as I can, as they can require a lot of effort to maintain and are often not necessary in well-written code (which I can't claim to write yet, as much as I may want to); I also find that they make the code harder to read when too abundant, and my first instinct when looking at heavily-commented code is often to get rid of all the redundant comments -but that's just me.

But that doesn't mean you shouldn't write comments! However, rather than explaining what the code is doing (which the code itself already does), comments should be used to explain things that cannot be inferred or understood simply by reading the code; for example, telling why one method or technique was used in this particular case rather than another.

As an example, in the snippet below, the comment is only really useful because of how the variable n is named. If it were named nbSteps, or maybe currentValue (I'm not very good with names either, but I'm trying to improve), the comment would then be completely unnecessary. But if you see a reference to n somewhere else is the code, you might think "mmmh, what was n again? was it the size of the matrix? the number of steps I can take? or maybe the number of elements in the queue? who knows!".

// value of current cell
int n = matrix[i][j];

Similarly, the comment // The function returns false if is not a valid position doesn't tell us anything that we don't already know from simply reading the method's name.

This takes us to variable names, and naming in general. Giving your variables, classes and methods meaningful names (that actually tell you what they are or what they do) is probably the most essential thing to making code easy to read. Again, most of your comments simply tell the reader what one things or another is or does, which accomplish very little.

Shortened or abbreviated names like curr or q don't really have any benefit over longer, clearer ones, other than making the very first time you type the name a tiny bit shorter. Renaming them to current and queue for example will make the code easier to read later, and you shouldn't be worried about typing a few more characters each time; let you IDE help you and do its job!

There is probably a lot more to say about comments and maybe JavaDoc, but I think the above outlines enough of the basics to improve your code commenting habits.

I've rewritten your Node class using the above guidelines while keeping its functionality, structure and meaning as close to the original as possible :

Node.java

// queue node used in BFS
public class Node {
    private final int x; // x-coordinate of a cell in matrix
    private final int y; // y-coordinate of a cell in matrix
    private final Node parent; // maintain a parent node for printing final path

    public Node(final int x, final int y, final Node parent) {
        this.x = x;
        this.y = y;
        this.parent = parent;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    public Node getParent() {
        return parent;
    }

    @Override
    public String toString() {
        return '(' + x + ", " + y + ')';
    }
}

I've made all the fields in Node.java private final and added getters, as you don't ever change them after setting them in the constructor. You also used both single and double quotes for characters in your toString() method, which I've corrected.

While trying to refactor and make improvements to your Solution class, I found I was having a hard time understanding how it's meant to be used; at first, I thought it was a static class, from which you call findPath(), which would return... a Node? But you have a mix of static and non-static methods and variables, and use them within one another, but have no constructur...

Also, getShortestPath() returns a list that will only ever contain the node added to it by findPath(), which is simply the lower-right node (or destination, as you call it), so you only ever get the destination and never the actual full path.

I would maybe suggest you refactor this portion of your code and submit it for another review, as -unless I'm mistaken- I do not believe it is actually functional right now.

Some additional things...

I strongly recommend you use the final keyword everywhere you can; method arguments, local variables... it indicates that the value you're assigning to the variable should not and will not change after it's been assigned. In other words, a final variable can only be assigned once, e.g. you can't do final int x = 45; x = 25;.

Again, there's more to say about the use of final, but if you'd like to know more I recommend you take a look at this answer : https://softwareengineering.stackexchange.com/questions/98691/excessive-use-final-keyword-in-java.

I also notice that you didn't include imports in your questions; you should add those next time you ask a question on CodeReview.

You declared an overload for toString() in your Node class, why not use it for the value provided to key? That way, if you ever want to change that key, you can simply change the toString() method instead of having to change it in two places. Or you could create another method just for this purpose, to keep the toString() for displaying a nicely formatted string, while the other one would be a simplified version of your toString(), used solely for generating the 'keys'.

Another thing; you declare shortestPathList as a LinkedList<String>, and getShortestPath() returns a List<String>. I'd suggest you either change shortestPathList's type to a List<String>, or change the method's return type, as it will only ever return shortestPathList. You can find more information about this topic here : https://stackoverflow.com/questions/12321177/arraylist-or-list-declaration-in-java.

If I've made any mistakes or if you disagree with anything I've said, please refrain from throwing your keyboard at me but do point them out to me so that I can improve my own skills and knowledge!


Program to index a book Indexing a book. Write a program that reads in a text file from
  standard input and compiles an alphabetical index of which words
  appear on which lines, as in the following input. Ignore case and
  punctuation. For each word maintain a list of location on which it
  appears. Try to use HashTable  and/or HashMap class (of java.util).

I have used a HashMap to store the line numbers for each word where it appears. Can this program be made better?

Index.java

package java_assignments.beg_assignment5;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;

public class Index {
    public Index(Readable text) {
        Scanner sc = new Scanner(text);
        occurences = new HashMap<String, ArrayList<Integer>>();

        int lineNo = 1;
        try {
            while (sc.hasNextLine()) {
                String line = sc.nextLine();
                String[] words = line.split("\\W+");
                for (String word : words) {
                    word = word.toLowerCase();
                    ArrayList<Integer> list = occurences.get(word);
                    if (list == null) {
                        list = new ArrayList<>();
                        list.add(lineNo);
                    } else {
                        list.add(lineNo);
                    }
                    occurences.put(word, list);
                }
                lineNo++;
            }
        } finally {
            sc.close();
        }
    }

    public String toString() {
        return occurences.toString();
    }

    private Map<String, ArrayList<Integer>> occurences;
}

BookIndexer.java

package java_assignments.beg_assignment5;

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.InputStreamReader;

public class BookIndexer {
    public static void main(String[] args) {
        try {
            BufferedReader br;
            if (args.length == 0) {
                br = new BufferedReader(new InputStreamReader(System.in));
            } else {
                br = new BufferedReader(new FileReader(args[0]));
            }
            String index_str = new Index(br).toString();
            System.out.println(index_str);
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }
    }

}

Using a try-with-resources

You're correctly closing your Scanner at the end of the method in a finally block, so there can be no resource leaks.

However, starting with Java 7, you can simply use the try-with-resources construct to make this easier:

try (Scanner sc = new Scanner(text)) {
    // ...
}

Reading words with lines

You're using a Scanner to read each line and then you are splitting the line on non word characters, i.e. everything that is not [a-zA-Z_0-9].

This can be a problem: what if you encounter a word that has a dash or a quote? You will wrongly split it. It would be better to split around a whitespace character, i.e. \s.

Also, you're currently using a lineNo variable to hold the current line number. You could use the built-in LineNumberReader that already maintains a line number. You can access it with getLineNumber().

Code structure

Your declaration of

private Map<String, ArrayList<Integer>> occurences;

is located at the bottom of the class. Generally, instance variables are found at the top instead so that you can see directly what the class has as instance variables.

You're currently using two classes: one for the main part and one to find the occurences. It introduces a problem: the constructor does too much work. In fact, the constructor of Index does all the work. It would be better to refactor this into a method properly named after what it does. We could introduce a method populateOccurences whose goal would be to create the occurences map.

Also, I don't think the Index class is really that necessary: the more a code is simple, the better it is to maintain it. In this case, this class really contains a single method, which to populate the occurences map. It would be easier to not have that class and simply have a method

private static Map<String, List<Integer>> getOccurencesMap(Reader text) throws IOException

inside the main class that would return the map.

Also, don't name your variables index_str: use camel-case, as indexStr.

Handling exceptions

When you're reading a text from a file, you're not directly catching the FileNotFoundException, instead you're letting the main method do it:

try {
    BufferedReader br;
    if (args.length == 0) {
        br = new BufferedReader(new InputStreamReader(System.in));
    } else {
        br = new BufferedReader(new FileReader(args[0]));
    }
    // ...
} catch (FileNotFoundException e) {
    e.printStackTrace();
}

This create a coupling between the method and what it reads from. Instead, it would be best to delegate that to a method dedicated to returning the Reader to read:

private static Reader getReader(String[] args) {
    if (args.length == 0) {
        return new BufferedReader(new InputStreamReader(System.in));
    } else {
        try {
            return new BufferedReader(new FileReader(args[0]));
        } catch (FileNotFoundException e) {
            throw new IllegalArgumentException("The given file does not exist.", e);
        }
    }
}

Note two things:

The catch (FileNotFoundException e) is done inside the else part: that is the only part of the code responsible for reading a file, so it must be the only part of the code for handling a FileNotFoundException.
A custom IllegalArgumentException is re-thrown to indicate that the file wasn't found. This runtime exception wraps the initial FileNotFoundException to have a proper stacktrace but it hides that from the surrounding code.

Lowercasing Strings

Be very careful when lowercasing / uppercasing Strings in Java. This depends on the locale. By default, Java will use the locale of the current JVM, which is your system locale (by default). If you were to read a Turkish text on a server in France, you might have inconsistencies and hard to understand bugs! It is preferable to use a locale when doing those operations

word = word.toLowerCase(Locale.ROOT);

Using Java 8 constructs

Your code updating the Map holding the line numbers for each word reads line

ArrayList<Integer> list = occurences.get(word);
if (list == null) {
    list = new ArrayList<>();
    list.add(lineNo);
} else {
    list.add(lineNo);
}
occurences.put(word, list);

Let alone the fact that you could drop the else clause and have list.add(lineNo); after the if (which would remove this little duplication), you could use the method computeIfAbsent that will get the value for a specified key or if there is no value, set it with an initial value based on the given mapping function. In this case, you can simply have

occurences.computeIfAbsent(word, k -> new ArrayList<>()).add(lineNo);

If the current word is not in the map, a new ArrayList will be created and returned, otherwise the current list for that word will be returned. Then, on this instance, we add the current line number.

Beginning with Java 8, a BufferedReader also has a useful lines() method that returns a Stream<String> of the lines. Instead of looping with a for, we could make that a Stream pipeline. This is what it would look like:

Make a Stream of the lines: this is done by calling lines() on the BufferedReader.
Flat map each line into a Stream of its words: this can done by using a method reference: Pattern.compile("\\s+")::splitAsStream. This creates a Pattern around the whitespace characters delimiter and splits each given String into a Stream<String> using splitAsStream. The :: operator creates the method-reference. Flat mapping is done by calling flatMap from the Stream API.
Map each word as lowercase: this can be done by using the lamda expression w -> w.toLowerCase(Locale.ROOT), fed to the map method of the pipeline
Collect that into a Map having the word as key and the line numbers as value: this can be done with the built-in Collectors.groupingBy collector, where the classifier returns the current word. All values mapped to the same word are collected using a downstream collector, which in this case would map, using Collectors.mapping, each line number into a downstream list (with Collectors.toList()).

Into code, it would look like:

try (LineNumberReader reader = new LineNumberReader(text)) {
    return reader.lines()
                 .flatMap(Pattern.compile("\\s+")::splitAsStream)
                 .map(w -> w.toLowerCase(Locale.ROOT))
                 .collect(Collectors.groupingBy(
                     w -> w,
                     Collectors.mapping(w -> reader.getLineNumber(), Collectors.toList())
                 ));
}

Of course, you can't run this in parallel.

Putting it all together

With all this, this is what you could have

public class BookIndexer {

    public static void main(String[] args) throws IOException {
        Reader br = getReader(args);
        String indexStr = getOccurencesMap(br).toString();
        System.out.println(indexStr);
    }

    private static Reader getReader(String[] args) {
        if (args.length == 0) {
            return new BufferedReader(new InputStreamReader(System.in));
        } else {
            try {
                return new BufferedReader(new FileReader(args[0]));
            } catch (FileNotFoundException e) {
                throw new IllegalArgumentException("The given file does not exist.", e);
            }
        }
    }

    private static Map<String, List<Integer>> getOccurencesMap(Reader text) throws IOException {
        try (LineNumberReader reader = new LineNumberReader(text)) {
            return reader.lines()
                         .flatMap(Pattern.compile("\\s+")::splitAsStream)
                         .map(w -> w.toLowerCase(Locale.ROOT))
                         .collect(Collectors.groupingBy(
                             w -> w,
                             Collectors.mapping(w -> reader.getLineNumber(), Collectors.toList())
                         ));
        }
    }

}


Finding the maximum of a given list of data in GNU Assembly x86 (32 bit) I am following the book Programming from the Ground Up and as an answer to a question in the Use the Concepts section of chapter 4:

  Convert the maximum program given in the Section called Finding a Maximum
  Value in Chapter 3 so that it is a function which takes a pointer to several values
  and returns their maximum. Write a program that calls maximum with 3
  different lists, and returns the result of the last one as the program’s exit status
  code.

I wrote the following code:

maxfunc.s

# NAME:     maxfunc.s
# PURPOSE:  A modular approach of finding the maximum of lists

    .section .data
    # The data section has three lists for testing, change the code
    # highlighted in the text section to change the list passed.
data_1:
    .long 1, 2, 3, 4, 5, 6, 7, 8, 9, 0
data_2:
    .long 23, 45, 62, 13, 87, 54, 0
data_3:
    .long 1, 2, 3, 3, 3, 7, 6, 5, 8, 1, 1, 0

# VARIABLES:
# 1. %edi - Will be our index variable, to access the items
# 2. %ebx - The element we're looking at currently
# 3. %eax - The maximum value we've found so far
# 4. %ecx - To store the address of the list

    .section .text
    .globl _start
_start:
    # Push the address of the list we want to find the maximum of
    pushl   $data_3
    #             ^
    #             +---> Change this to select your list

    call    maximum
    addl    $4, %esp    # Reset the stack

    movl    %eax, %ebx
    movl    $1, %eax
    int     $0x80

    .type   maximum, @function
maximum:
    # Setup
    pushl   %ebp
    movl    %esp, %ebp

    # The initial setup:
    # Get the address of the list
    # Set the index to 0
    # Get the first item from the list
    # The first item is currently the largest
    movl    $0, %edi
    movl    8(%ebp), %ecx
    movl    (%ecx, %edi, 4), %ebx
    movl    %ebx, %eax

max_loop:
    cmpl    $0, %ebx
    je      exit_loop

    incl    %edi
    movl    (%ecx, %edi, 4), %ebx

    cmpl    %eax, %ebx
    jle     max_loop

    # %ebx is greater than %eax, therefore, update max
    movl    %ebx, %eax
    jmp     max_loop

exit_loop:
    # Tear down
    movl    %ebp, %esp
    popl    %ebp
    ret

It compiles and works as intended. I'm compiling it as a 32 bit binary on my 64 bit machine, so that I can follow the book.

I'd like suggestions on the proper use of conventions, and everything in general.

In general I try to stick to the naming conventions of the registers themselves. Although, I have been criticized for it because many programmers think of these conventions as archaic. But I think it helps reading the code.

  General-Purpose Registers (GPR) - 16-bit naming conventions[edit] The    8 GPRs are:

  Accumulator register (AX). Used in arithmetic operations.
  Counter register (CX). Used in shift/rotate instructions and loops.
  Data register (DX). Used in arithmetic operations and I/O operations.
  Base register (BX). Used as a pointer to data (located in segment register    DS, when in segmented mode).
  Stack Pointer register (SP). Pointer to the top of the stack.
  Stack Base Pointer register (BP). Used to point    to the base of the stack.
  Source Index register (SI). Used as a    pointer to a source in stream operations.
  Destination Index register (DI). Used as a pointer to a destination in stream operations.

Cited from: https://en.wikibooks.org/wiki/X86_Assembly/X86_Architecture

.section .data
data_1:
    .long 1, 3, 4, 5, 6, 7, 8, 9, 0
data_2:
    .long 23, 45, 62, 13, 87, 54, 0
data_3:
    .long 1, 3, 7, 6, 5, 8, 1, 1, 0

.section .text
.globl _start
.type maximum, @function
.type exit, @function

_start:
    movl    $data_3, %ebx
    call    maximum

exit:
    movl    %eax, %ebx
    movl    $1, %eax
    int     $0x80

maximum:
    movl    (%ebx), %edx # notice i did not index address here.
    movl    %edx, %eax
    movl    $1, %esi

max_loop:
    cmpl    $0, %edx
    je      exit_loop
    movl    (%ebx, %esi, 4), %edx
    cmpl    %edx, %eax
    cmovng   %edx, %eax
    incl    %esi
    jmp     max_loop

exit_loop:
    ret

Instead of passing the args on the stack, I opted to pass the address in the base register. AMD64 defines way different calling conventions than the original 32bit stuff. If this were a non trivial program we'd definitely want to adhere to the sysv calling conventions which define which registers may be overwritten or must be preserved between calls, stack alignment, etc. In the maximum function we could use a cmov__ instruction if we wanted to conditionally move the data register to the accumulator (current max) if it was greater.

The next book to get if you want to learn a great deal more about assembly AFTER programming from the ground up is Professional Assembly Language.

Linus Torvalds has criticized cmov instructions because of the out of order execution nature of modern processors. Which the Professional Assembly Language book explains in detail.

http://yarchive.net/comp/linux/cmov.html


Insert multiple rows into a SQL table I've written the following C# code to insert rows (that a user enters from a web application) into a SQL database.  How does this code look?  Is there a better, more efficient way to accomplish the task?

public void UpdateDeviceStatus(string[] deviceId, byte[] status, string userId, string[] remarks, DateTime dateTurnin)
{
        if (deviceId.Length != status.Length || status.Length != remarks.Length)
            throw new ArgumentOutOfRangeException("Invalid arguments passed to UpdateDeviceStatus: deviceId, status, and remarks must contain the same number of entries.");
        if (deviceId.Length == 0)
            throw new ArgumentOutOfRangeException("UpdateDeviceStatus expects to update status for at least one deviceId, but an empty array was passed in.");

        // Build the SQL statement
        StringBuilder sbSql = new StringBuilder();
        sbSql.Append(@"INSERT INTO AT_Event_History(deviceId,parentCode,statusCode,remarks,userId,whenEntered) VALUES");
        for (int i = 0; i < deviceId.Length; i++)
        {
            string values = string.Format("({0},0,{1},{2},{3},{4}),",
                new string[] { "@deviceId" + i.ToString(), "@statusCode" + i.ToString(), "@remarks" + i.ToString(), "@userId", "@whenEntered" });
            sbSql.Append(values);
        }
        string sql = sbSql.ToString();
        sql = sql.TrimEnd(','); // remove the trailing comma ','

        Database db = EnterpriseLibraryContainer.Current.GetInstance<Database>("AssetTrackConnection");

        DbCommand command = db.GetSqlStringCommand(sql);
        command.CommandType = CommandType.Text;
        command.CommandText = sql;

        // Add in parameters
        db.AddInParameter(command, "@userId", DbType.AnsiString, userId);
        db.AddInParameter(command, "@whenEntered", DbType.Date, dateTurnin);
        for (int j = 0; j < deviceId.Length; j++)
        {
            db.AddInParameter(command, "@deviceId" + j.ToString(), DbType.Guid, new Guid(deviceId[j]));
            db.AddInParameter(command, "@statusCode" + j.ToString(), DbType.Byte, status[j]);
            db.AddInParameter(command, "@remarks" + j.ToString(), DbType.AnsiString, remarks[j]);
        }

        // Execute the statement.
        db.ExecuteNonQuery(command);
}

As you can see, I am looping to add db parameters to hold the value for each row.  I think that there may be a better way to do this.

You are building a dynamic insert statement with parametrized values.  This works and there's nothing wrong with this method.  It may even be the best method for your circumstance.  It works well when your table is "small".  I have a rather large database which grows monotonically.  We keep adding rows and never remove any.  When your table grows beyond a certain point that is specific to your table design, inserts get very slow.  At this point, you will want to consider using the SqlBulkCopy class.  You insert the values into a DataTable and then do a bulk insert.  This is much faster because it uses a SQL Server specific method of loading data faster.  The SqlBulkCopy link has sample code.


Multi threaded application to send large no of emails I need to build a application which sends a lot of emails. These emails are based on a table. Basic idea is, I grab a bunch of records from database. Create MailMessage (System.Net.Mail) and send it. Since there are a lot of mails to be sent, I am thinking about developing a multi-threaded application.

    public void Run()
    {
        var mailItems = MailItemDataProvider.GetLiveMailItems();
        if (mailItems.Count > 0)
        {
            var maxThreads = 5;
            var totalItems = mailItems.Count;
            int skipper = totalItems / (maxThreads - 1);
            if (skipper == 0)
                skipper = totalItems;

            for (var i = 0; i < totalItems; i += skipper)
            {
                var itemsLot = mailItems.Skip(i).Take(skipper).ToList();
                Task.Factory.StartNew(() => ProcessMails(itemsLot, ThreadPriority.Highest));
            }
        }
    }

It basically grabs a thousand records from the database and divide them into five chunks. The chunks are passed to the mail processor. The above function is triggered from a timer (Timer_Elapsed event) configured to run every minute. This is actually part of Windows Service. Mail processor is like this:

    protected void ProcessMails(IEnumerable<MailItem> mailItems)
    {
        foreach (var mailItem in mailItems)
        {
            bool errorFound;
            var mail = CreateMailMessage(mailItem, out errorFound);
            // returns (System.Net.Mail) MailMessage object

            if (!errorFound)
            {
                if (Mailer.SendEmail(mail))
                {
                    mailItem.StatusId = Status.Sent;
                }
                else
                {
                    mailItem.StatusId = Status.Failed;
                    mailItem.FailMessage = "Error sending email.";
                }
            }
            else
            {
                mailItem.StatusId = Status.Failed;
            }
        }
        UpdateMailItems(mailItems);
        // this updates mail sent status in the database
    }

In my test environment, it is working fine. I have tested for 100,000 emails.
Following are my concerns in the function that is processed by the threads:

What will happen when SMTP slows down (for some unknown reason)? I start up five threads, but it will add up five more before the first five complete. How many threads can my server handle? (I know it depends on actual resources available, but practically, how will it perform?)
I have database update inside the process. Is that good idea?
Is there a chance that some threads will never finish? Do I need to make sure that threads are completed? How do I detect the ever running threads and kill them?

I am new to multi threading. Please review my code. Any comments or suggestion are welcome.

Instead of rolling your own version of a parallel for each loop, you should use Parallel.ForEach(). It will make your code simpler, avoid the possibility of bugs in your Run() method and you will still be able to configure the number of threads used (through ParallelOptions).

  I start up five threads, but it will add up five more before the first five complete.

If I understand your code correctly, the only way this could happen is if you called Run() for a second time. If that's the case, it might make sense to use the producer-consumer pattern using BlockingCollection<T> and GetConsumingEnumerable() (or GetConsumingPartitioner() if you decide to switch to Parallel.ForEach()).

  How many threads can my server handle?

There is no way for us to answer that. You will have to perform your own measurements.

  I have database update inside the process. Is that good idea?

I don't see why would that cause any significant issues.

  Is there a chance that some threads will never finish?

There is no way for us to know that without seeing the code you didn't show.


std::accumulate alternative for the C programming language I've come up with some simple implementation for algorithms in C and more specifically for std::accumulate alternate since I need something like it recently. Can you provide some review on the approach?
#include <stdio.h>
#include <string.h>

#define ARRSIZE(X) (sizeof(X) / sizeof(X[0]))

#define ACCUMULATE(RET, FIRST, LAST, INIT, SUM_CB)      \
    do {                                                \
        RET = INIT;                                     \
        while (FIRST != LAST){                          \
            RET = SUM_CB(RET, *FIRST);                  \
            FIRST++;                                    \
        }                                               \
    } while(0);

/**
 * example usage
 * sum integers and sum strings by some delimiter
*/
struct str_t
{
    char data[512];
};

struct str_t sumStr(struct str_t a, struct str_t b)
{
    struct str_t ret = {0};
    strcat(ret.data, a.data);
    strcat(ret.data, "--");
    strcat(ret.data, b.data);
    return ret;
}

struct A { int a, b, c; };

struct A sumA(struct A a, struct A b) {
    struct A ret ;
    ret.a = a.a + b.a;
    ret.b = a.b + b.b;
    ret.c = a.c + b.c;
    return ret;
}

int main()
{
    unsigned long i;

    struct A sumvec[10];
    struct str_t sumstr[10];
    memset(sumstr, 0, sizeof(sumstr));

    for(i=0; i < ARRSIZE(sumvec); i++) {
        sumvec[i].a = i;
        sumvec[i].b = i+1;
        sumvec[i].c = i+2;
        snprintf(sumstr[i].data, sizeof(sumstr[i].data), "%d", i);
    }

    for(i=0; i < ARRSIZE(sumvec); i++) {
        printf("[%d][%d][%d]\r\n", sumvec[i].a ,sumvec[i].b,sumvec[i].c);
    }

    struct A ret = {sumvec[0].a, sumvec[0].b, sumvec[0].c};
    struct A* pBegin = &sumvec[1];
    struct A* pEnd = &sumvec[ARRSIZE(sumvec)];

    ACCUMULATE(ret, pBegin, pEnd,  ret, sumA)

    printf("SUM: [%d][%d][%d]\r\n", ret.a, ret.b, ret.c);

    struct str_t retstr;
    strcat(retstr.data, sumstr[0].data);

    struct str_t *pBegs = &sumstr[1];
    struct str_t *pEnds = &sumstr[ARRSIZE(sumstr)];

    ACCUMULATE(retstr, pBegs, pEnds, retstr, sumStr);

    printf("SUM STR: [%s]\r\n", retstr.data);
    return 0;
}

When using the do { ... } while (0) idiom for macros, we don't generally include the terminating semicolon in the definition.  That's so we can use it like a function call:
if (condition)
    ACCUMULATE(result, a, b, 0, add);
else
    ACCUMULATE(result, a, c, 10, add);

There's a typo in the ARRSIZE() macro: where we protect X from misbinding - should be (X)[0], not (X[0]):
#define ARRSIZE(X) (sizeof (X) / sizeof (X)[0])

Also, be careful with this macro when its argument isn't a whole, completely defined array.  In particular, it will be wrong when the argument is a variable-length (dynamically-allocated) array, or when the argument is a pointer (perhaps the array decayed to pointer - that can easily catch you out!).
In the sample usage:

Identifiers ending in _t are reserved for future use by the standard library, so str_t should be renamed.
It's a shame, after being so careful to use snprintf() avoid overflow, that we  we then use unchecked strcat();strcat();strcat(); in sumStr()  - why not snprintf() there, too?
A lot of needless parens around the arguments to sizeof.  Just write sizeof sumstr instead of sizeof(sumstr), for example.


Longest words in dictionary that can be constructed from a list of letters The problem is to find the longest words in a dictionary of legal words that can be constructed from a given list of letters.

For example:

$ ./scrabble dictionary.txt i g h l p r a
argil glair grail graph hilar laigh phial pilar ralph

More details here.

I've already solved this problem in Python (see this).

To brush up my C++ I decided to reproduce the exact same algorithm in C++. I learned C++ as "C with classes" and never learned to use the STL properly, so I usually end up writing more code than necessary to accomplish simple things.

This is what I came up with for this problem:

#include <algorithm>
#include <fstream>
#include <iostream>
#include <vector>

using namespace std;

bool can_make(string word, vector<char> letters);
int get_max_len(vector<string> words);

int main(int argc, char* argv[])
{
    // Grab the dictionary.
    fstream dict_file(argv[1]);
    string in;
    vector<string> dict_words;
    while(getline(dict_file, in)) dict_words.push_back(in);
    dict_file.close();

    // Get the list of input letters.
    vector<char> letters;
    for (int i = 2; i < argc; i++)
        letters.push_back(argv[i][0]);

    // Get all words.
    vector<string> all_words;
    vector<string>::iterator it;
    for (it = dict_words.begin(); it < dict_words.end(); it++)
        if (can_make(*it, letters))
            all_words.push_back(*it);

    // Get longest words.
    int max_len = get_max_len(all_words);
    vector<string> longest_words;
    for (it = all_words.begin(); it < all_words.end(); it++)
        if ((*it).length() == max_len)
            longest_words.push_back(*it);

    // Print the result.
    for (it = longest_words.begin(); it < longest_words.end(); it++)
        cout << *it << " ";
    cout << endl;

    return 0;
}

bool can_make(string word, vector<char> letters)
{
    /*
        Return true if the word <word> can be generated by all letters in <letters>
        and only the letters in <letters>.
    */

    if (word.length() > letters.size()) return false;

    vector<char> l(letters);
    vector<char> word_letters(word.c_str(), word.c_str()+word.length());
    vector<char>::iterator it, loc;

    // Iterate through <word_letters>. If a letter in <word_letters> also
    // appears in <letters>, remove it. Otherwise, return false.
    for (it = word_letters.begin(); it < word_letters.end(); it++) {
        loc = find(letters.begin(), letters.end(), *it);
        if (loc == letters.end()) return false;
        letters.erase(loc);
    }

    return true;
}

int get_max_len(vector<string> words)
{
    /*
        Return the length of the longest word(s) in <words>.
    */

    int max_len = 0;
    vector<string>::iterator it;
    for (it = words.begin(); it < words.end(); it++)
        if ((*it).length() > max_len)
            max_len = (*it).length();

    return max_len;
}

Is this idiomatic C++?
How can I make this code more concise?
How can I make it more correct?
What C++11 features can I use here to make the code even shorter?
What features have I used here that I should avoid?

Preface

I am not sure the algorithm you have chosen is the best one. Sorting the letters, or using a frequency table of the letters, are obvious alternatives. However, this is code review, and not algorithm review.

So here we go:

Iterators

Don't use < on iterators, use != on iterators. < only works on bidirectional iterators, != works on all iterators. std::list<> is an example where < won't work.

Use ++it instead of it++. Theoretically, the latter has to return a temporary copy of the original, which is additional overhead when the iterator is a class. Practically, it won't make much of a difference (if any) as most compilers will inline and optimize. Still, it's one of the best signals whether someone is a true C++ expert or a converted C-programmer :P .

So

for (it = dict_words.begin(); it < dict_words.end(); it++)

Will become:

for (it = dict_words.begin(); it != dict_words.end(); ++it)

Predefined algorithms

Many of your one-line loops can be replaced with algorithms from <algorithm>.

In this small case, I wouldn't bother as it wouldn't help to make your code more clear or shorter.

On the other hand, experimenting with them in this case will make you more comfortable with them. With the introduction of lambdas in C++11, those algorithms will become much more common in new code!

C++11

Due to the fact that C++11 hasn't been around long yet (and compilers don't fully implement it yet), idiomatic C++11 will change over the next couple years. So take this section with a grain of salt.

That being said, use auto for the type of it. That part of C++11 has become idiomatic already.

E.g.:

vector<string>::iterator it;
for (it = dict_words.begin(); it != dict_words.end(); ++it)

will become:

for (auto it = dict_words.begin(); it != dict_words.end(); ++it)

EDIT: oops, forgot about the range-based for in C++11. See other answers for that. I'm not that well versed in C++11 yet, but I believe it would look like this:

for (auto it : dict_words)

More functions

Most of your code is in one long function, called main. Try to split it up.

Once you start inserting empty lines and comment headings to clarify what the block does, it's usually a good candidate to turn into a separate function.

E.g. this block of code (limited by empty lines, with a comment):

// Grab the dictionary.
fstream dict_file(argv[1]);
string in;
vector<string> dict_words;
while(getline(dict_file, in)) dict_words.push_back(in);
dict_file.close();

Can be replaced with this function:

void grab_dictionary(const char* filename, vector<string>& dict_words)
{
    fstream dict_file(filename);
    string in;
    while(getline(dict_file, in))
       dict_words.push_back(in);
}

Usage from main:

vector<string> dict_words;
grab_dictionary(argv[1], dict_words);

Additional benefit:

The comment is no longer needed - usually it has become the name of the function!
There is no need for dict_file.close() as the destructor will close the file at the right time.


Code Chef RAINBOW Palindrome Problem

  Determine whether an array is "Rainbow", i.e. it has the following
  structure:

  First a1 elements equal 1.
  Next a2 elements equal 2.
  Next a3 elements equal 3.
  Next a4 elements equal 4.
  Next a5 elements equal 5.
  Next a6 elements equal 6.
  Next a7 elements equal 7.
  Next a6 elements equal 6.
  Next a5 elements equal 5.
  Next a4 elements equal 4.
  Next a3 elements equal 3.
  Next a2 elements equal 2.
  Next a1 elements equal 1.

  ai can be any positive integer. There are no other elements in array.

Solution

(defn palindrome? [items]
  "Efficient algorithm: loops through only (array-size/2)"
  (loop [i 0
         j (-> items count dec)]
    (cond (>= i j)
          true
          (not= (nth items i)
                (nth items j))
          false
          :else
          (recur (inc i) (dec j)))))

(let [test-cases-count (Integer/parseInt (read-line))
      test-cases       (for [i (range test-cases-count)]
                         (let [nums-count (read-line)
                               nums-temp (-> (read-line) (clojure.string/split #"\s+"))
                               nums (map #(Integer/parseInt %) nums-temp)]
                           nums))]
  (for [t test-cases]
    (if (palindrome? t)
      (println "Yes")
      (println "No"))))

Not able to figure out how to optimize this.

(defn palindrome? [items]
  "Efficient algorithm: loops through only (array-size/2)"
     ...
         (not= (nth items i)
               (nth items j))

nth is efficient on vectors and other things that support random indexing. On sequences, which you're passing in right now, it's O(n) per lookup since it has to walk the list.

Changing your test cases to use vectors should help (use mapv instead of map):

(let [...
      nums (mapv #(Integer/parseInt %) nums-temp)]

(def example (concat (range 1000) (reverse (range 1000))))
(count example)   ; 2000
(def examplev (vec example))
(count examplev)  ; 2000

(dotimes [_ 3] (time (palindrome? example)))
; "Elapsed time: 45.167334 msecs"
; "Elapsed time: 44.568282 msecs"
; "Elapsed time: 45.304136 msecs"

(dotimes [_ 3] (time (palindrome? examplev)))
; "Elapsed time: 0.317649 msecs"
; "Elapsed time: 0.272229 msecs"
; "Elapsed time: 0.30044 msecs"


Simple Calculator to add/subtract/multiply/divide 2 inputs This is for an assignment in my robotics class so the requirements I needed to meet was that it had to be in a while loop and the input numbers needed to be float point. I just want to make sure that I'm not using any bad practices and if there are any areas that can be improved.
while (True):
    # Gets operator, converts to int and breaks if its not a number
    try: operator = int(input("Choose an Operator:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\nInput (1/2/3/4):"))
    except:
        print('Invalid Input: Choose A Valid Operation')
        break

    # Checks the operator is within 1 and 4
    if operator > 4 or operator < 1:
        print('Invalid Input: Choose A Valid Operation')
        break

    # Gets first and second number, converts numbers to float and breaks if its not a number
    try: firstnum = float(input('First Number:'))
    except:
        print('Invalid Input: Not a valid number')
        break
    try: secondnum = float(input('Second Number:'))
    except:
        print('Invalid Input: Not a valid number')
        break

    if operator == 1:
        print('{0} + {1} ='.format(firstnum, secondnum), end =" ")
        print(firstnum + secondnum)
        break
    if operator == 2:
        print('{0} - {1} ='.format(firstnum, secondnum), end =" ")
        print(firstnum - secondnum)
        break
    if operator == 3:
        print('{0} * {1} ='.format(firstnum, secondnum), end =" ")
        print(firstnum * secondnum)
        break
    if operator == 4:
        print('{0} / {1} ='.format(firstnum, secondnum), end =" ")
        print(firstnum / secondnum)
        break

Formatting strings in Python
foo = 5
print('{foo}'.format(foo))

This is a nice way to format string, But there is a short and cleaner way to format strings in Python 3 known as the f-String. The problem with str.format() is that if you have multiple parameters. Your string starts to look a little werid.For example
first_name = "Eric"
last_name = "Idle"
age = 74
profession = "comedian"
affiliation = "Monty Python"
print(("Hello, {first_name} {last_name}. You are {age}. " +
        "You are a {profession}. You were a member of {affiliation}.") \
        .format(first_name=first_name, last_name=last_name, age=age, \
               profession=profession, affiliation=affiliation))

With f-strings. The same would look like.
print(f"Hello, {first_name} {last_name}. You are {age}. " +
      f"You are a {profession}. You were a member of {affiliation}.")

This is why you should prefer using f-strings.
eval in Python
When you want to perform the arithmetic part, I see that you have used a thread of if-statements. But we can use Python's eval function to make life much easier. We simple place the operator entered by the user un between the two numbers.
old
if operator == 1:
    print('{0} + {1} ='.format(firstnum, secondnum), end =" ")
    print(firstnum + secondnum)
    break
if operator == 2:
    print('{0} - {1} ='.format(firstnum, secondnum), end =" ")
    print(firstnum - secondnum)
    break
if operator == 3:
    print('{0} * {1} ='.format(firstnum, secondnum), end =" ")
    print(firstnum * secondnum)
    break
if operator == 4:
    print('{0} / {1} ='.format(firstnum, secondnum), end =" ")
    print(firstnum / secondnum)
    break

We ask the user to enter the operation the want directly , for example

Choose an operator: +

Or if you want to stick to using numbers.
operators = operators = {'+':1,'-':2,'*':3,'/':4}
I have gone with the 1st method. You can use whatever you prefer to
new
print(f"{firstnum} {operator} {secondnum} = {float(eval(firstnum + operator + secondnum))}")

The new code using eval would like
operators = ['+','-','*','/']
while (True):
    # Gets operator, converts to int and breaks if its not a number
    try: operator = input("Choose an Operator:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\nInput\n(+,-,*,/):")
    except:
        print('Invalid Input: Choose A Valid Operation')
        break

    # Checks the operator is within 1 and 4
    if operator not in operators:
        print('Invalid Input: Choose A Valid Operation')
        break

    # Gets first and second number, converts numbers to float and breaks if its not a number
    try: firstnum = input('First Number:')
    except:
        print('Invalid Input: Not a valid number')
        break
    try: secondnum = input('Second Number:')
    except:
        print('Invalid Input: Not a valid number')
        break

    print(f"{firstnum} {operator} {secondnum} = {float(eval(firstnum + operator + secondnum))}")

With that simple change, I removed about 20 lines of code into one. Making it much more readable
Clearing the screen
It would be nicer to see a clear terminal after finishing one job. There are many ways to clear the terminal in python. A common way for windows users is
os.system('cls')

But this way is considered bad as it is quite expensive. You can get away by simply
print(chr(27) + "[2J")

Adding this to the start of each iteration in the while(True): loop will give a better experience to the user.
Waiting for input
Lastly, adding this line of code to the end will pause on the current text before going to the next one.
input("Press any key to continue...")


Validation of uploading images and adding records to the database I am doing a php project with the codeigniter framework. In my controller, I have a function that does the following.

Uploads an image to the server
Add a row to the database. The data would be retrieved from the form that calls the controller.

The best approach might be to put these two things in separate functions, but it has to be atomic (If either the upload or the insert fails, the other should not complete) so I decided to put these in one function.

First here are the helper functions I used:

// I created this so I can use it an an argument to functions
function is_empty($var) {
    return empty ( $var );
}

// has the functionality of array_map and array_filter
function array_map_filter($orig, $pred, $apply) {
    $result = array ();
    foreach ( $orig as $key => $value ) {
        if ($pred ( $value )) {
            $result [$key] = $apply ( $value );
        }
    }
    return $result;
}

// checks the status of the new uploaded file saved in a temporary folder. The error code and full path of the file is taken from the $_FILES variable
function validate_file($full_path, $error_code) {
    if (is_file ( $full_path )) {
        throw new Exception ( 'This file already exist.' );
    }
    if ($error_code != 0) {
        throw new Exception ( $error_code );
    }
}

Here is my function.

public function add() {
    if (empty ( $_POST )) {
        $data ['title'] = 'add form';
        $this->load->view ( 'templates/header', $data );
        $this->load->view ( 'content/add' );
        return;
    }
    $errors = array ();
    try {
        $errors = array_map_filter ( $_POST, 'is_empty', function ($val) {
            return "no value";
        } );
        if (empty ( $errors )) {
            $root = $_SERVER ['DOCUMENT_ROOT'];
            $upload_path = $root . 'images/';
            $file_field = 'image_path';
            $file = $_FILES [$file_field];
            $file_name = $file ['name'];
            $full_path = $upload_path . $file_name;
            // a file should already be uploaded in a tmp folder. The following function checks the status of the upload
            // throws an exception, so the rest cannot execute
            validate_file ( $full_path, $file ['error'] );
            $rest = array (
            // ...
            // throws an exception if the record already exist in the database, so the rest cannot execute
            $this->gmodel->insert ( 'neo_content', array_merge ( $_POST, $rest ), array (
                    'title' => $_POST ['title']
            ) );
            move_uploaded_file ( $file ['tmp_name'], $full_path );
        }
    } catch ( Exception $e ) {
        // catch everything and add them to the $errors array
        $errors ['error'] = $e->getMessage ();
    }
    header ( 'Content-Type: application/json' );
    // send to ajax
    echo json_encode ( $errors );
}

As you've noticed, the way I tried to insure the operation is atomic is to throw an exception so I can guarantee the program will not continue to the next part of the code. I've read that doing this is generally bad because users giving wrong inputs should always be expected and the flow of the program should not be interrupted because of that. But I believe I can use exceptions in this case because I wish to preserve atomicity.

Is my code wrong? Are there better ways of doing this? How can I make this code better?

validate_file

I would get rid of your validate_file function. Generally, I'm a big supporter of creating as many functions as possible, as it makes code easier to read, reuse, and test.

But in this case, there are two problems: 1) It handles two somewhat different functionalities (does the file already exist, and was there an error on upload) and 2) It's named poorly (likely because of point 1)): It's not really clear what the function does, because "validate" is such a general term. If I read something like that, I would at a minimum also expect a file extension check, a mime type check, maybe a check for an acceptable file name, a check if it's really an image, etc.

File Check

It doesn't seem that you have any actual checks on the uploaded file, so it seems that a user could just upload eg a PHP file and gain code execution.

You should definitely check the file extension, and if you can move the uploaded file outside the web root, and perform some kind of check on the file itself.

Error Handling and Function signature

Throwing exceptions is fine, but I wouldn't return a string error message on failure (the "no value" return). It's difficult to handle (and it leaves you with the unnecessary if error check, and nesting which makes it a bit hard to follow when what is happening).

Your function signature also doesn't seem all that clear. add is quite generic (uploadImage would be better), and it doesn't accept arguments, but works on global variables directly (I would pass them on).

The return value is also quite unclear, and the function itself sometimes prints stuff, which makes it difficult to use. I would let the calling code show responses (if you must, the alternative would be to show all responses in this function, instead of sometimes returning a string).


Pulse-width modulation module Module for generating a PWM signal. The req_value_i input gets a duration value of the signal. Furthermore, the module can be stopped by deassertion of the enable_i input.
`timescale 1ns / 1ps

 module pwm #
(
    parameter integer PWM_COUNTER_WIDTH = 8
)
(
    input  logic                             clk_i,
    input  logic                             s_rst_n_i,
    input  logic                             enable_i,

    input  logic [PWM_COUNTER_WIDTH - 1 : 0] req_value_i,

    output logic                             channel_o
    );

    logic [PWM_COUNTER_WIDTH - 1 : 0] counter;

    always_comb begin
        channel_o = (req_value_i != counter) ? 1'h1 : 1'h0;
    end

    always_ff @ (posedge clk_i) begin
        if (1'h0 == s_rst_n_i) begin
            counter <= {PWM_COUNTER_WIDTH{1'h0}};
        end
        else if (1'h1 == enable_i) begin
            counter <= counter + 1'h1;
        end
    end

endmodule

The layout of your code is consistent and easy to read.  I don't see any functional problems.
One consideration for all digital logic is whether you can tolerate glitches on your outputs.  If your PWM output is directly driving a load such as a power inverter, you likely want to avoid glitches.  To avoid glitches, change your combinational output to a registered output:
always_ff @ (posedge clk_i) begin
    if (1'h0 == s_rst_n_i) begin
        channel_o <= 1'h0;
    end
    else begin
        channel_o <= (req_value_i != counter);
    end
end

I also simplified the expression by removing the ternary code ? 1'h1 : 1'h0.  But, this is just a matter of coding style preference.
Another consideration is whether to use a synchronous reset (as you have) or an asynchronous reset.  The advantage of asynchronous is that you don't need to have the clock toggling in order to reset your logic.  For asynchronous, you would use:
always_ff @ (posedge clk_i or negedge s_rst_n_i) begin


Function for obtaining the 3 latest patient vital signs I'm just putting together a small project, checking to see if this would be the fastest/cleanest way to write out a function that grabs the 3 latest vital signs.

Edit: I am thinking GetRecentVitalSigns should be in the domain model.

Also, if I wanted to grab all the vital signs, I would just call the VitalSigns list, but I was thinking I could make that as private and create a GetAllVitalSigns, but that seems redundant. Any advice would be excellent (on anything related).

public class Patient
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime AdmissionDate { get; set; }
    public List<string> Allergies { get; set; }
    public List<VitalSign> VitalSigns { get; set; }

    public List<VitalSign> GetRecentVitalSigns()
    {
        if(VitalSigns.Count > 0)
        {
            var results = VitalSigns
                .Take(3)
                .OrderBy(x => x.DateTimeChecked)
                .ToList();

            return results;
        }
        return null;
    }
}
public class VitalSign
{
    public decimal BodyTemperature { get; set; }
    public int Pulse { get; set; }
    public int BloodPressure { get; set; }
    public int RespiratoryRate { get; set; }
    public DateTime DateTimeChecked { get; set; }
}

New structure

public class Patient
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime AdmissionDate { get; set; }
    public List<string> Allergies { get; set; }
    private List<VitalSign> vitalSigns;

    private IEnumerable<VitalSign> VitalSigns
    {
        get { return vitalSigns; }
    }

    public IEnumerable<VitalSign> GetRecentVitalSigns(int vitalSignsCount = 3)
    {
        var recentVitalSigns = new IEnumerable<VitalSign>();
        if (recentVitalSigns.Any())
        {
            recentVitalSigns = vitalSigns
                .OrderBy(vs => vs.DateTimeChecked)
                .Take(vitalSignsCount);
        }
        return recentVitalSigns;
    }
}
public class VitalSign
{
    public decimal BodyTemperature { get; set; }
    public int Pulse { get; set; }
    public int BloodPressure { get; set; }
    public int RespiratoryRate { get; set; }
    public DateTime DateTimeChecked { get; set; }
}

I would just call the VitalSigns list, but I was thinking I could make that as private and create a GetAllVitalSigns, but that seems redundant.

That depends on what you want the users of your class to allow. As it is, any user can modify everything about the contents of your objects. Depending on how are you going to use them, that might not be a good idea.
Consider whether it would make sense to make (some of) your setters private or whether VitalSigns should be more something like:
private List<VitalSign> vitalSigns;
public IEnumerable<VitalSign> VitalSigns { get { return vitalSigns; } }

Now, to the GetRecentVitalSigns() method:
public List<VitalSign> GetRecentVitalSigns()
{
    if(VitalSigns.Count > 0)
    {
        var results = VitalSigns
            .Take(3)
            .OrderBy(x => x.DateTimeChecked)
            .ToList();

        return results;
    }
    return null;
}

This seems wrong to me on several levels:

Most importantly, I think it's wrong, because you first take the first 3 items from the list and then order those 3 items. You should do it the other way around: first sort and then take the last 3 based on that.
Depending on how many items are in the list, sorting the whole collection every time might be too slow. But it's hard to suggest the best solution without knowing how the class is going to be used.

You return null when there are no results. That's a bad practice, when you want to represent an empty collection, use empty collection, not null. It will make both the code that uses the result and the code in the method simpler and avoids NullReferenceException bugs.

You return List. I see no good reason for that here, just return IEnumerable. If the caller of this method wants a List that they can modify, they can easily do that by calling ToList() themselves.

When it makes sense, make even the variables in your lambdas at least a bit descriptive. For example, using vs for a VitalSign is better then just x.

Consider whether the number of items to return should be configurable. Are you sure 3 is always the right number? You could still make it the default. (On the other hand, don't do this if you expect that 3 will be always the right number.)

This means that I would probably write the method like this:
public IEnumerable<VitalSign> GetRecentVitalSigns(int vitalSignsCount = 3)
{
    return VitalSigns
        .OrderByDescending(vs => vs.DateTimeChecked)
        .Take(vitalSignsCount);
}

Also:
public decimal BodyTemperature { get; set; }

Yes. I think that most people would use double here (decimal is just for money, right?), but decimal is probably the right choice here. (Assuming the temperature is entered by humans. If it it were somehow read directly from the thermometer, then double would make sense.)