Printing removed items using lambdas and streams I'm struggling to make my lambdas readable.

I've seen various approaches.  Below are three different examples that all do the same thing.

Forgive the example: I suspect there are better actual solutions to this problem. I'm only concerned with readability (and ultimately supportability).

/** Output any items that are present in "existing" but not in "updated" */
private static void outputRemovedItems(List<String> existing, List<String> updated) {
    // #1 simple for-loop
    for (String item : existing) {
        if (!updated.contains(item)) {
            System.out.println("item " + item + " removed");
        }
    }

    // #2 fluid lambdas
    existing.stream()
        .filter(item -> !updated.contains(item))
        .forEach(item -> System.out.println("item " + item + " removed"));

    // #3 awful-looking middle-ground: functions split out
    Predicate<String> onlyRemovedItems = item -> !updated.contains(item);
    Consumer<String> outputItems = item -> System.out.println("item " + item + " removed");
    existing.stream().filter(onlyRemovedItems).forEach(outputItems);
}

Personally, I find the lambda approaches more difficult to read than the simple for-loop: they make me think too much, which detracts from the readability.  So this is stopping me from using them in actual production code.
The more complex the operation, the worse my lambdas seem to look!

Does anyone have any tips to make my lambdas more readable?

Or.. do people from a functional programming background actually find the lambdas above easier to read than the loop?

Your "fluid" example is fine, well structured, the newlines are consistent, and is in line with code styles that I have seen and like. It is perhaps a bit too soon to say it follows "best practice", but it does look right.

// #2 fluid lambdas
existing.stream()
    .filter(item -> !updated.contains(item))
    .forEach(item -> System.out.println("item " + item + " removed"));

The "middle-ground" approach you have is immediately "identical" to the fluid approach, but in this context is less readable, even if functionally identical. It does have advantages when your function is, for example, a method parameter, or an injected constant (like a custom comparator, or something). Since the code is all localized to a single method, though, the need to separate the declaration and use of the function is simply not there.

My only concern with your fluid approach, and it is a small one, is that you reuse the item placeholder in two contexts. While it is logical, in this case, to use item, I find it is typically better to have unique variable names at each stage in the pipeline, perhaps:

// #2 fluid lambdas
existing.stream()
    .filter(item -> !updated.contains(item))
    .forEach(removed -> System.out.println("item " + removed + " removed"));

Additionally, using printf helps too:

    .forEach(removed -> System.out.printf("item %s removed\n", removed));

One lase comment, I have found that the more I use Java 8 features the easier it gets to establish the right mindset when reading the code. You may just find that it grows on you, and your statement "I find the lambda approaches more difficult to read than the simple for-loop" is no longer true. Also, consider this:

// #2 fluid lambdas
existing.stream()
    .parallel()
    .filter(item -> !updated.contains(item))
    .forEach(removed -> System.out.printf("item %s removed\n", removed));

and consider doing that in "the simple for loop".


Extracting specific rows and columns from a CSV file I have written a function to selectively extract data from a file.
I want to be able to extract only from a certain line and only given rows.

Would convert this function into a generator improve the overhead when I need to process large files?

import itertools
import csv

def data_extraction(filename,start_line,lenght,span_start,span_end):
    with open(filename, "r") as myfile:
        file_= csv.reader(myfile, delimiter=' ')  #extracts data from .txt as lines
        return (x for x in [filter(lambda a: a != '', row[span_start:span_end]) \
        for row in itertools.islice(file_, start_line, lenght)])

Use round parenthesis for generators

Also x for x in was unnecessary:

return (filter(lambda a: a != '', row[span_start:span_end]) \
    for row in itertools.islice(file_, start_line, lenght))

If you use Python 2 you should use itertools.ifilter because it returns a generator while filter returns a list.

The functions is pretty clear overall, I suggest you space your argument list as according to PEP8 conventions. Also investigate in easier to remember argument formats like f(file, line_range, inline_range) where two tuples replace 4 arguments.


MD5 Brute Force Algorithm I am coding an Objective-C MD5 hash "decrypter". Since you cannot decrypt such a hash function, I am approaching it with a brute force algorithm, that tries every single password combination until it found the correct one or not.

I would highly appreciate any help to improve the algorithm to make it even faster. I am currently computing approx. 28,000 guesses per second on my iMac and 7,000 on an iPhone 6. I know that 7,000 guesses are quite ridiculous compared to the performance a tool like hashcat has (2.2 billion guesses per second on my iMac), nonetheless I want a mobile MD5 decryption tool on my iPhone.

-(void)brute_sequentialWithMaxLength:(int)length
{
   iterateFurther = YES;

   char * buf = malloc(length + 1);

   for (int i=1; i <= length; i++)
   {
     memset(buf, 0, length+1);
     [self brute_implWithChar:buf andIndex:0 andDepth:i];
   }

  free(buf);
}

-(void)brute_implWithChar:(char*)str andIndex:(int)index andDepth:(int)max_depth
{

 for (int i=0; i < [pm_maskArray count] && iterateFurther == YES;i++)
 {
    alph = [[pm_maskArray objectAtIndex:i] characterAtIndex:0];

    str[index] = alph;

    if (index == max_depth - 1)
    {

         if ([[self makeMD5StringFromString:[NSString stringWithFormat:@"%s",str]] isEqualToString:pm_hashString])
         {
           NSLog(@"Found");
           iterateFurther = NO;
         }

    }
    else
    {
      [self brute_implWithChar:str andIndex:index+1 andDepth:max_depth];
    }

 }
}

rolfl's answer pretty well explains why this isn't really a realistically reasonable approach to cracking passwords.  I'm going to set that answer aside, ignore it, and address some of the specific ways in which this code could be improved if it were part of a more realistic program.

As a start, this code has one massive, massive problem: What happens when we actually find a match?

NSLog("Found");

And that's it.

We don't log the password.  We don't post a notification, return it, tell a delegate.  We don't save it as a local variable.  We just post a borderline useless log statement claiming we found it.

And what's worse?  WE KEEP CALCULATING!

We're done with this call to brute_implWithChar, but as soon as it returns, the loop in brute_sequentialWithMaxLength: is on another iteration and we jump right back in brute_implWithChar..., and go through all of that, and then return and do it all again, etc.

This is a major problem which should be addressed.

We can speed this up by using fast enumeration in brute_implWithChar...

But to do that, let's first talk about: pm_maskArray.  You don't show this anywhere, but I can only assume it's an array of NSString objects.  I'm going to recommend instead that we use an array of NSNumber objects.

And why NSNumber?  Because NSNumber can also hold characters:

NSNumber *theCapitalLetterC = @'C';
char theCStyleCapicalCChar = [theCapitalLetterC characterValue];

So... presuming we've built pm_maskArray into an appropriate array of NSNumber objects, our loop is now going to look something more like this:

for (NSNumber *aChar in pm_maskArray) {
    char alph = [aChar characterValue];

    // all your checking stuff
    // when/if a match is found, we can:
    break;
    // to stop the loop and return executing at the first line after the loop
    // or in this case we can also:
    return;
    // to exit the method entirely.
}

As it stands, this forin loop will already allow you SEVERAL more guesses per second, as it will send a message to the pm_maskArray about once for every 16 times your version of the code was sending the message (approximately).

And looking back to my first point, we probably want to change this method to return an NSString object, rather than void.  Then, if we find a match, we return it, otherwise, we return nil.

I'm also going to rename this method to strip the "and"s out of it, as they're inappropriate.

So if our method looks like this:

-(NSString *)brute_implWithChar:(char*)str index:(int)index depth:(int)depth
// returns matching password or nil

Then the loop in which we call it can look like this:

NSString *crackedPassword = nil;
while ((crackedPassword = [self brute_implWithChar:but index:0 depth:i]) == nil
    && i <= length) {
    ++i;
}
return crackedPassword;

Now, on every unsuccessful call of brute_implWithChar:index:depth: for which we don't find a password, nil is returned and assigned to crackedPassword.  If crackedPassword == nil evaluates to true AND i is still not larger than length, the loop will iterate again.

Meanwhile, the loop will stop iterating as soon as crackedPassword evaluates to anything non-nil.  And since we're returning the password from the inner method, the outer method can return it to its caller as well.  If the password isn't found before i <= length evaluates false, then nil will be returned, indicating that the password wasn't found--otherwise, the password is returned.


Inserting persons in file into SQLite I've just started working with SQLite in C# to test various features of an application. In building my first SQLite example I wanted to insert a large .csv into a table (Person) with two columns, A and B. The .csv file is 50MB in size and contains multiple rows of the same data ("abc, def").

I wrote the following code, but I think this is fairly inefficient and am looking for advice on a better method(s) to optimize this code to read the file and finally insert it into SQLite.

Regex splitRx = new Regex(@",\s*", RegexOptions.Compiled);
ArrayList al = new ArrayList();

using (StreamReader sr = new StreamReader(@"c:\Temp\test.csv"))
{
    string line = null;
    int ln = 0;
    while ((line = sr.ReadLine()) != null)
    {
        string[] fields = splitRx.Split(line);
        if (fields.Length != 2)
        {
            Console.WriteLine("Invalid Input on line:" + ln);
            continue;
        }
        ln++;
        al.Add(fields);
    }
}

using (var conn = new SQLiteConnection(@"Data Source=C:\Temp\test.sqlite"))
{
    conn.Open();

    using (var cmd = new SQLiteCommand(conn))
    {
        using (var transaction = conn.BeginTransaction())
        {
            foreach (string[] sa in al)
            {
                cmd.CommandText =
                    "INSERT INTO Person (FirstName, LastName) VALUES ('" + sa[0] + "', '" + sa[1] + "');";
                cmd.ExecuteNonQuery();
            }

            transaction.Commit();
        }
    }

    conn.Close();
}

This seems quite fine, in no small part thanks to the use of using (...) { ... } blocks to manage resources.

Use prepared statements

The biggest improvement would be to use prepared statements for inserting the records. See the example in the docs.

A small bug

You have a small bug in the line counting:

    if (fields.Length != 2)
    {
        Console.WriteLine("Invalid Input on line:" + ln);
        continue;
    }
    ln++;

Since you only increment the counter after the check on fields.length,
the line number will be correct for the first invalid line,
and off by one for the next invalid line, off by 2 for the one after,
and so on.
In other words, to keep the count correct, you need to do it before the if.

Use the var keyword consistently

You used the var keyword almost everywhere, except here:

using (StreamReader sr = new StreamReader(@"c:\Temp\test.csv"))

You can simplify with var sr = ...


PHP breadcrumb class I am fresh at OOP and I am curious if the code below is object oriented and can be improved:

class cTitleBreadcrumb {

public function __construct($sAction, $sItem){
    $this->sAction = $sAction;
    $this->sItem = $sItem;
}

public function displayAction(){
    if($this->checkUrlAction($this->sAction) === true){
        return $this->sAction;
    }
}
public function displayItem(){
    if($this->checkUrlItem($this->sItem) === true){
        return $this->sItem;
    }
}
private function checkUrlAction($sAction){

    if($sAction == 'insert' || $sAction == 'view' || $sAction == 'update' || $sAction == 'delete'){
        return true;
    }
}

private function checkUrlItem($sItem){
    if($sItem == 'imagelist' || $sItem == 'mkdir' || $sItem == 'rdir'){
        return true;
    }
    else{
        $objShowPDO = new mShowPDO();
        $result = $objShowPDO->allTables();
        while($array = $result->fetch()){
            if($array[0] == $this->sItem){
                return true;
            }
        }
    }
}

$objTitleBreadcrumb = new cTitleBreadcrumb($sAction,$sItem);

echo ucfirst($objTitleBreadcrumb->displayAction());

echo ucfirst($objTitleBreadcrumb->displayItem());

You are not always returning anything

private function checkUrlAction($sAction) {
    if(true){
        return true;
    }

    //but else?
}

Dependency injection

$objShowPDO = new mShowPDO(); is bad a mShowPDO instance should by injected via method or constructor argument see dependency injection topics

private function checkUrlItem(mShowPDO $objShowPDO, $sItem){
    if($sItem == 'imagelist' || $sItem == 'mkdir' || $sItem == 'rdir'){
        return true;
    }

    //redundand else removed

    $result = $objShowPDO->allTables(); //maybe you can inject only the result
    while($array = $result->fetch()){
        if($array[0] == $this->sItem){
            return true;
        }
    }

    return false;
}

Hard coding

The checklists are really hard coded (long if with OR relations), try use some container for them (an array and use in_array() for example).


Generate String with Random Consonants and Vowels Purpose

This problem comes from this dailyProgrammer subreddit challenge.

The task is to take an input of C / cs and V / vs and to replace Cs with random consonants and Vs with random vowels (English alphabet) while keeping the case the same. Y is not considered a vowel in this case.

Example:

CcVvv => BgAoi
ccVVV => zqUUE

Implementation

For this case, I've put all methods in the same class - in reality, I might split these methods out into different classes.

Edit: I forgot to throw an exception in generateRandomString when the isValidCharacter method is called and returns false.

public class RandomCharacterReplacer {
  public static final char C = 'C';
  public static final char V = 'V';
  public static final List<Character> CONSONANTS = new ArrayList<>(
      Arrays.asList('B', 'C', 'D', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'X', 'Y', 'Z')
  );
  public static final List<Character> VOWELS = new ArrayList<>(
      Arrays.asList(
          'A', 'E', 'I', 'O', 'U'
      )
  );

  private static boolean isValidCharacter(final char c) {
    final char upperCaseChar = Character.toUpperCase(c);
    return upperCaseChar == C || upperCaseChar == V;
  }

  public static AlphabetCharacterCase returnAlphabetCharacterCase(final char c) {
    if (!Character.isAlphabetic(c)) {
      throw new RuntimeException("character is non-alphabetic");
    }

    if (Character.isUpperCase(c)) {
      return AlphabetCharacterCase.UPPER;
    }

    if (Character.isLowerCase(c)) {
      return AlphabetCharacterCase.LOWER;
    }

    throw new RuntimeException("unexpected character case");
  }

  public static AlphabetCharacterType returnAlphabetCharacterType(final char c) {
    if (!Character.isAlphabetic(c)) {
      throw new RuntimeException("character is non-alphabetic");
    }

    final char upperCaseChar = Character.toUpperCase(c);
    if (upperCaseChar == C) {
      return AlphabetCharacterType.CONSONANT;
    }

    if (upperCaseChar == V) {
      return AlphabetCharacterType.VOWEL;
    }

    throw new RuntimeException("unexpected character type");
  }

  public static char returnRandomCharacter(final List<Character> characterList) {
    if (characterList.isEmpty()) {
      throw new IllegalArgumentException("character list must be non-empty");
    }
    final Random random = new Random();
    return characterList.get(random.nextInt(characterList.size() - 1));
  }

  public static List<Character> returnAlphabetCharacterTypeList(final AlphabetCharacterType characterType) {
    switch (characterType) {
      case CONSONANT: {
        return CONSONANTS;
      }

      case VOWEL: {
        return VOWELS;
      }

      default: {
        throw new RuntimeException("unexpected character type");
      }
    }
  }

  public static char generateRandomCharacter(final AlphabetCharacterCase alphabetCharacterCase, final AlphabetCharacterType characterType) {
    final List<Character> characters = returnAlphabetCharacterTypeList(characterType);
    switch (alphabetCharacterCase) {
      case UPPER: {
        return returnRandomCharacter(characters);
      }

      case LOWER: {
        return Character.toLowerCase(returnRandomCharacter(characters));
      }

      default: {
        throw new RuntimeException("unexpected character case");
      }
    }
  }

  public static String generateRandomString(final String input) {
    final char[] chars = input.toCharArray();
    final StringBuilder stringBuilder = new StringBuilder();
    for (final char c : chars) {
      if (isValidCharacter(c)) {
        final AlphabetCharacterCase alphabetCharacterCase = returnAlphabetCharacterCase(c);
        final AlphabetCharacterType alphabetCharacterType = returnAlphabetCharacterType(c);
        stringBuilder.append(generateRandomCharacter(alphabetCharacterCase, alphabetCharacterType));
      }
    }

    return stringBuilder.toString();
  }
}

I think if I were doing this, I'd do it...somewhat (a lot?) differently. My immediate reaction would be to do something on this general order (using C++ syntax instead of Java, but I'm pretty sure the same basic idea should work in Java about the same way):

char gen_rand(std::string const &input) {
    // A quick and dirty method, for now.
    return input[rand() % input.size()];
}

static const string lower_c = "bcdfghjklmnpqrstvwxyz";
static const string upper_c = "BCDFGHJKLMNPQRSTVWXYZ";
static const string lower_v = "aeiou";
static const string upper_v = "AEIOU";

string genRandomString(std::string const &input) {
    string output; // I suppose needs to be a StringBuilder in Java

    for (char ch : input)
        switch (ch) {
        case 'c': output.push_back(gen_rand(lower_c));    break;
        case 'C': output.push_back(gen_rand(upper_c));    break;
        case 'v': output.push_back(gen_rand(lower_v));    break;
        case 'V': output.push_back(gen_rand(upper_v));    break;
        default: throw std::runtime_error("Unexpected character in input");
        }
     return output;
 }

If I'm allowed to speak in generalities, your code seems (to me) to spend a lot of effort on issues that are almost incidental to the question at hand (e.g., 37 lines just to decide that c, C, v and V refer to lower- and upper-case consonants and vowels). At least as I see things, the fact that the case of the input corresponds to the case of the output is really mostly incidental--they could just as well be a, b, c and d instead.

Another option that seems obvious to me, would be to use a map to take an input character and retrieve the collection of characters to choose from for that input. This is probably more work than it's worth for only 4 fixed inputs, but if you might have a lot of inputs, or (especially) if you want to support those inputs being specified at run time (e.g., reading them from a configuration file) a map becomes much more attractive.

Bottom line: it's a lot simpler and more flexible to just treat the mapping from input character to action as entirely arbitrary rather than go to a lot of work to classify the inputs as upper/lower case.


Finding unimplemented tasks on Rosetta Code using Requests This is a Find unimplemented tasks task from Rosetta Code:

  Given the name of a language on Rosetta Code, find all tasks which are
  not implemented in that language.

There is already a solution that uses urllib, but I decided to add another one using Requests. I've never worked with this library before and would like my code to get reviewed. Any comments are welcome.

"""
Given the name of a language on Rosetta Code,
finds all tasks which are not implemented in that language.
"""
from functools import partial
from operator import itemgetter
from typing import (Dict,
                    Iterator,
                    Union)

import requests

URL = 'http://www.rosettacode.org/mw/api.php'
REQUEST_PARAMETERS = dict(action='query',
                          list='categorymembers',
                          cmlimit=500,
                          rawcontinue=True,
                          format='json')

def unimplemented_tasks(language: str,
                        *,
                        url: str,
                        request_params: Dict[str, Union[str, int, bool]]
                        ) -> Iterator[str]:
    """Yields all unimplemented tasks for a specified language"""
    with requests.Session() as session:
        tasks = partial(tasks_titles,
                        session=session,
                        url=url,
                        **request_params)
        all_tasks = tasks(cmtitle='Category:Programming_Tasks')
        language_tasks = set(tasks(cmtitle=f'Category:{language}'))
        for task in all_tasks:
            if task not in language_tasks:
                yield task

def tasks_titles(*,
                 session: requests.Session,
                 url: str,
                 **params: Union[str, int, bool]) -> Iterator[str]:
    """Yields tasks names for a specified category"""
    while True:
        request = session.get(url, params=params)
        data = request.json()
        yield from map(itemgetter('title'), data['query']['categorymembers'])
        query_continue = data.get('query-continue')
        if query_continue is None:
            return
        params.update(query_continue['categorymembers'])

if __name__ == '__main__':
    tasks = unimplemented_tasks('Python',
                                url=URL,
                                request_params=REQUEST_PARAMETERS)
    print(*tasks, sep='\n')

Current output for Python:

Calendar - for "REAL" programmers
Catmull–Clark subdivision surface
Checkpoint synchronization
Colour pinstripe/Printer
Compile-time calculation
Constrained genericity
Create an object at a given address
Deconvolution/2D+
Factorial base numbers indexing permutations of a collection
Fixed length records
Four is the number of letters in the ...
Function prototype
Hilbert curve
Parametric polymorphism
Pattern matching
Peano curve
Pinstripe/Printer
Rendezvous
Rosetta Code/Find bare lang tags
Start from a main routine
Suffixation of decimal numbers
Video display modes
Zeckendorf arithmetic

LGTM. Ship it!

There were two minor nits that seemed slightly off:

The use of * in the signature to force kw args is maybe overused, as it covers args that are pretty mandatory.
The print(*tasks, sep='\n') would more naturally be handled by this idiom, which is reusable outside of print(): print('\n'.join(tasks))


Single-account-per-email verification This is a function that checks that when users register into the site, they only create one single account per email. The little twist is that all emails in the DB are hashed with unique salts (=Input2). Because hashing the new mail input with all unique salts (=STEP 2) will be horribly slow, I keep a "diggest" version of emails (=Input3) in my DB. The diggest emails are a shorter and useless version of the emails. For example, John@gmail.com becomes jo@gm.com. The goal of that is diggest version is to reduce the number of emails to check in the DB (=STEP 1).

This function works pretty well but I'm afraid it will became very slow when DB becomes bigger.

Here is the code of the portion relevant to the problem:

$Requete1 = mysql_query("SELECT Input2 as email_test, Salt as salt FROM utilisateurs WHERE Input3='$email_diggest'");

    //Step 1
    if(mysql_num_rows($Requete1) == 0)
    {
        echo "No problem baby";
    }

    //Step 2
    while($row = mysql_fetch_assoc($Requete1))
    {
        $email_test = $row['email_test'];
        $salt = $row['salt'];

        $hash_email = base64_encode(pbkdf2($Email, $salt, 100000, 32));

        if($email_test == $hash_email)
        {
            echo "An account already exist with that email";
        }else
        {
            echo "No problem baby";
        }
    }

I have to agree with Corbin, while I would be annoyed if my email address got leaked, I would not be overly concerned. Spam filters are pretty decent now, and without a password all they can do is spam me. Or, if I have a gmail account, WoW hackers can find more loopholes that allows them to spam FROM my address. I don't even play WoW anymore, I wish they'd just leave me alone -.- Anyways...

This looks like sample code, so there's not much I can help with. I would suggest that you be sure to escape early whenever possible. For instance, in the first if statement. This ensures there is no wasted processes parsing code that ins't necessary. Also, not sure how much of a difference this would make, but I believe your email comparison should be absolute, not loose.

if( $email_test === $hash_email ) {

Only other thing I can think of is using extract() to get those array indices. But there's a couple of problems with that. First, its not always well liked by the community. Second, it dumps the entire array index list into the function's local variable scope, which is sometimes unnecessary and may lead to security issues. I only bring this up because if these are the only two fields in this table then there shouldn't be any issues here. As for speed, I'm not sure, you'd have to profile it.

The biggest thing here is premature optimization. Yes, its nice to plan ahead, but ask yourself this: How likely is it that your database will ever reach a size where this will become an issue? Every developer dreams that their creation will be the next biggest thing and will have millions of viewers, but the likelihood is slim. Plus, with regular maintenance and periodic deletion of inactive accounts will help ensure that the size, even in these extreme cases, is less likely to become a factor.


Hangman - my first Clojure code This is my first attempt at Clojure! Apart from all game related issues, how is my code? Is there anything I can do to make it more idiomatic?

(ns hangman.core
  (:import (java.net ServerSocket Socket SocketException)
           (java.io PrintWriter InputStreamReader BufferedReader))
  (:gen-class))

(defn guess [guess word]
  (apply str (map #(let [x (str %)] (if (.contains guess x) x "*")) word)))

(defn listen [port]
  (new ServerSocket port))

(def word "clojure")

(defn conn-handler [conn]
  (let [in (:in @conn)
        out (:out @conn)]
    (.println out "Welcome to this simple hangman game.")
    (def the-guess (atom ""))
    (loop [conn conn]
      (.println out (str "Guess the word: " (guess @the-guess word)))
      (.flush out)
      (let [g (str @the-guess (.readLine in))
            res (guess g word)]
        (if (= word res)
          (do
            (.println out res)
            (.println out "Correct!")
            (.flush out))
          (do
            (reset! the-guess g)
            (recur conn)))))))

(defn -main [& args]
  (println "Server running…")
  (with-open [server (listen 3456)]
    (loop []
      (let [client (.accept server)
          in (BufferedReader.(InputStreamReader.(.getInputStream client)))
          out (PrintWriter. (.getOutputStream client))
          conn (ref {:in in :out out})]
        (println "Client connected")
        (doto (Thread. #(conn-handler conn)) (.start))
        (recur)))))

A bunch of random observations

avoid def as a non-top level form e.g. in (def the-guess (atom "")), prefer let bindings
there's no need to use a ref for the connection, as it never changes after creation. Prefer a simple map, which also enables you to use plain destructuring instead of using let to extract its components:

(defn conn-handler [{:keys [in out] :as conn}] ...)
...
conn {:in in :out out}]
(println "Client connected")
(doto (Thread. #(conn-handler conn)) (.start))

instead of using interop to interact with the in/out streams you could use a more idiomatic (binding [*out* out *in* in] (println ...) (read-line))
you loop over the conn, but you always recur over the very same object. You can remove conn from the loop/recur forms
if you follow the binding approach as described above, but within the anonymous function that you use to initialize the thread, you could completely remove the conn name from the whole code:

(defn conn-handler [] (println "...") ...)

(doto (Thread. #(binding [*in* in *out* out] (conn-handler)) (.start))

there's no need to use an atom for the guess, just use plain recursion:

(loop [the-guess ""]
...
  (recur g))

The following code contains the revised conn-handler and -main functions, including all the suggestions above, plus the cosmetic change of using the threading macro -> instead of the doto form, which seems a bit clearer:

(defn conn-handler []
  (println "Welcome to this simple hangman game.")
  (loop [the-guess ""]
    (println (str "Guess the word: " (guess the-guess word)))
    (flush)
    (let [g (str the-guess (readLine))
          res (guess g word)]
      (if (= word res)
        (do
          (println res)
          (println "Correct!")
          (flush))
        (do
          (recur g))))))

(defn -main [& args]
  (println "Server running…")
  (with-open [server (listen 3456)]
    (while true
      (let [client (.accept server)
            in (BufferedReader.(InputStreamReader.(.getInputStream client)))
            out (PrintWriter. (.getOutputStream client))]
        (println "Client connected")
        (-> #(binding [*in*  in
                       *out* out]
               conn-handler)
            (Thread.)
            (.start))))))


Graph using adjacency list in Java This is the main code.  Here I'm joining vertices by edges.

node[] gra=new node[5];
{
    for (int i = 0; i < 5; i++)
        gra[i] = new node(i);  //I'm allocating memory for all the vertices.
}
void add_edge(int to,int from)  //to represent the vertice the edge is going and from is now self explanatory
{
    node new_vert=new node(to);
    node temp=gra[from];
    while(temp.next!=null)
        temp=temp.next;
    temp.next=new_vert;
}

Now to do this operation I'm using this loop in main method.

for(int i=0;i<5;i++)  //For simplicity I have taken 5 vertices
    {
        int n=sc.nextInt();                     //N is Number of edges from the vertices, for example in figure for vertex 0 the number of edges are 2.
        for(int j=0;j<n;j++)
        {
            int to=sc.nextInt();
            obj.add_edge(to,i);
        }
    }

Is my implementation correct or efficient enough so that I can use this code for further use?

Feel free to ask if you don't understand the code; I will try to explain.  I know I'm bad at explaining.  And please forgive my English.

Here is my node class

    class node
{
    int data;
    node next;
    node(int data)
    {
        this.data=data;
        this.next=null;
    }
}

Use Java LinkedList class

Almost always prefer built-in data structures to hand-rolled. For instance, your insertion routine takes O(n) time while LinkedList::addLast takes O(1) times.

Also, use an ArrayList instead of an array to hold the lists due to problems with generic arrays in Java.

Write a Graph class

A class offers a great opportunity to re-organize your code. It makes it easy to separate initialization and adding edges.

Correctness issues

Validate the input to addEdge. Throw an exception if either to or from is not in the appropriate range.

Style issues

indent and capitalize properly
write fewer comments: only when something need explaining
write shorter comments: use sentence fragments

import java.util.ArrayList;
import java.util.LinkedList;

public class Graph {
    private ArrayList<LinkedList<Integer>> adjacencyList;

    public Graph(int numberNodes) {
        adjacencyList = new ArrayList<LinkedList<Integer>>(numberNodes);
        for (int i = 0; i < numberNodes; i++) {
            adjacencyList.add(new LinkedList<Integer>());
        }
    }

    private boolean isValidNode(int index) {
        return index >= 0 && index < adjacencyList.size();
    }

    public void addEdge(int to, int from) {
        if (!isValidNode(to) || !isValidNode(from)) {
            throw new IndexOutOfBoundsException();
        }

        adjacencyList.get(from).addLast(to);
    }
}

import java.util.Scanner;

public class GraphRunner {
    public static void main(String args[]) {
        Scanner sc = new Scanner(System.in);
        Graph g = new Graph(5);

        for(int from=0; from < 5; from++) {
            int n=sc.nextInt();
            for(int j=0; j < n; j++) {
                int to=sc.nextInt();
                g.addEdge(to, from);
            }
        }
    }
}


Selection Sort Algorithm (Python) Selection Sort

  The selection sort algorithm sorts a list by finding the minimum
  element from the right unsorted part of the list and putting it at the
  left sorted part of the list. The algorithm maintains two sub-lists in
  a given input list.

  1) The sub-list which is already sorted.
  2) Remaining sub-list which
  is unsorted.

  In every iteration of selection sort, the minimum element from the
  unsorted sub-list is picked and moved to the sorted sub-list.

I've been trying to implement the Selection Sort algorithm using Python magic functions such as __iter__ and I'd appreciate it if you'd review the code for changes/improvements.

Code

"""
This class returns an ascending sorted integer list
for an input integer list using Selection Sort method.

Sorting:
- In-Place (space complexity O(1))
- Efficiency (time complexity O(N^2))
- Unstable Sort (Order of equal elements might change)

"""
class SelectionSort(object):
    """
    """
    def __init__(self, input_list:list)->list:
        self.input_list = input_list
        self.__iter__()

    def __iter__(self)->list:
        """
        Iterates through the list and swaps the min from the right side
        to sorted elements from the left side of the list.
        """

        # Is the length of the list
        input_length = len(self.input_list)

        # Iterates through the list to do the swapping
        for element_index in range(input_length - 1):

            min_index = element_index

            # Iterates through the list to find the min index
            for finder_index in range(element_index+1, input_length):
                if self.input_list[min_index] > self.input_list[finder_index]:
                    min_index = finder_index

            # Swaps the min value with the pointer value
            if element_index is not min_index:
                self.input_list[element_index], self.input_list[min_index] = self.input_list[min_index], self.input_list[element_index]

        print(self.input_list)
        return self.input_list

SelectionSort([10, 4, 82, 9, 23, -30, -45, -93, 23, 23, 23, 0, -1])

Solution from Geeks by Geeks

I'm not so sure about Sorting Stability, it says the following implementation is not stable. However, Selection Sort can be made stable:

import sys
A = [64, 25, 12, 22, 11]

for i in range(len(A)):

    min_index = i
    for j in range(i+1, len(A)):
        if A[min_index] > A[j]:
            min_index = j

    A[i], A[min_index] = A[min_index], A[i]

for i in range(len(A)):
    print("%d" %A[i])

Reference

Selection Sort (Geeks for Geeks)
Selection Sort (Wiki)

I agree with @Reinderien that this shouldn't be a class. You can see evidence for this in your constructor:

def __init__(self, input_list:list)->list:
    self.input_list = input_list
    self.__iter__()

You are constructing the object (and calling the constructor) simply to call self.__iter__(). There is no reason for the creation of an object here just to sort the list. If you needed to maintain some state between sorts or something (I'm not sure why you would though), then it may be appropriate.

I'll also point out, you're attempting to violate at least two "contracts" with your usage of __init__ and __iter__:

__init__ must return None:

  no non-None value may be returned by __init__(); doing so will cause a TypeError to be raised at runtime.

Now, you aren't actually returning anything, but your type hinting is saying that you are. If you're going to use type hinting, the hints should make it clearer what types are involved, not make false claims.
__iter__ should return an iterator:

  This method should return a new iterator object that can iterate over all the objects in the container

The problem is, you're returning a list, and a list isn't an iterator, it's an iterable (it has an iterator). This isn't just a theoretical problem. Note how this can bite you:

class T:
    def __iter__(self):
        return [1, 2, 3]

for n in T():
    print(n)

# TypeError: iter() returned non-iterator of type 'list'

Use of "dunder" methods can be helpful for writing clean code, but only if you aren't abusing them. Make sure to read the documentation and understand the purpose and contracts of methods before attempting to use them.

And on the subject of type hints, you could make use of a TypeVar to allow the type checker to see the consistency between the element types going in and out of your sorting function. After making your class into a standalone function, you basically have:

def selection_sort(input_list: list) -> list:

The problem with this is, it doesn't tell the checker what the relationship is between the types of elements in input_list and that of the list that selection_sort returns. This can lead to subtle issues where it won't be able to help you with types:

lst: List[int] = [1, 2, 3]
sorted_list = selection_sort(input_list)
x = sorted_list[0]  # It has no idea what type x is

You can fix this by introducing a TypeVar that tells it that the element type remains consistent. I'm also changing from using list to List since list doesn't seem to support generics yet:

from typing import List, TypeVar

T = TypeVar("T")

# The sort returns the same element type T that it received
def selection_sort(input_list: List[T]) -> List[T]:
    . . .

Now, it is able to infer the type of x, and can give you better completions and type warnings.


Project Euler #1 efficiency I've started solving Project Euler problems. Is the code completely optimized?

Question:

  If we list all the natural numbers below 10 that are multiples of 3 or
  5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below N.

My solution:

import java.util.LinkedList;
import java.util.Scanner;

public class Solution {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        int testCases = sc.nextInt();
        for (int i = 0; i < testCases; i++) {
            int input = sc.nextInt();
            System.out.println(calculate(input));
        }

    }

    public static boolean isMulThree(int input) {

        if (input % 3 == 0)
            return true;
        else
            return false;
    }

    public static boolean isMulFive(int input) {
        int val = input % 10;
        if (val == 5 || val == 0)
            return true;
        else
            return false;
    }

    public static int calculate(int input) {
        LinkedList<Integer> al = new LinkedList<>();
        int sums = 0;
        for (int i = 1; i < input; i++) {
            if (isMulThree(i) || isMulFive(i)) {
                al.add(i);

            }
        }
        for (int count : al) {
            sums = sums + count;
        }
        return sums;
    }
}

I had a general review primed up for this question, but other answers beat me to those more general aspects.

@TheKittyKat provided a good starting point on the most efficient means of calculating the sum of each sequence, and then subtracting the double-count

Now, the solution @TheKittyKat provides for each sequence, is, for example:

for (int i = 5; i < n; i += 5) {
    sum += i;
}

If we were to put this in a function:

public static int sumOfSequence(int value, int limit) {
    int sum = 0;
    for (int i = value; value < limit; i += value) {
        sum += i;
    }
    return sum;
}

So, having generalized that, we can do:

int total = sumOfSequence(3, N) + sumOfSequence(5, N) - sumOfSequence(15, N);

But, there's a trick, the function:

public static int sumOfSequence(int value, int limit) {
    int sum = 0;
    for (int i = value; value < limit; i += value) {
        sum += i;
    }
    return sum;
}

can be reduced to a single \$O(1)\$ operation using the math behind a Arithmetic Progression / sum-of-sequence

This boils down to the fact that the sum of a sequence:

$$
1k + 2k + ... + (n-1)k + nk  = \tfrac{n(1k + nk)}{2}.
$$

The equation:

$$
\tfrac{n(1k + nk)}{2}
$$

can be rewritten as:

$$
\tfrac{kn(1 + n)}{2}
$$

Now, either \$n\$ or \$(1 + n)\$ is even, and any even number times any other even number, is even, so you can always safely halve it.

Thus, the function can be reduced to:

public static int sumOfSequence(int value, int limit) {
    final int count = (limit - 1) / value;
    return value * count * ( count + 1) / 2;
}

So, the end result, is:

public static int sumOfSequence(int value, int limit) {
    final int count = (limit - 1) / value;
    return value * count * ( count + 1) / 2;
}

public static void main(String[] args) {

    int limit = 1000;

    int sum = -sumOfSequence(15, limit) + sumOfSequence(3, limit) + sumOfSequence(5, limit);
    System.out.println(sum);
}


Random string + encrypt/decrypt Are there any security flaws in what I plan to do?

I need to store the following in my DB:

a random string to act as a salt for encrypting a password
the encrypted password that used the salt in #1

Here's the PHP code I have to accomplish the above tasks:

<?php
    function decrypt($string, $encryption_key = '')
    {
        $initialization_vector = get_initialization_vector();

        // Convert hexadecimal data into binary representation
        $string = hex2bin($string);

        // See: http://php.net/manual/en/mcrypt.ciphers.php
        return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_128, $encryption_key, $string, MCRYPT_MODE_ECB, $initialization_vector));
    }

    function encrypt($string, $encryption_key = '')
    {
        $initialization_vector = get_initialization_vector();

        // See: http://php.net/manual/en/mcrypt.ciphers.php
        $string = mcrypt_encrypt(MCRYPT_RIJNDAEL_128, $encryption_key, $string, MCRYPT_MODE_ECB, $initialization_vector);

        // Convert binary data into hexadecimal representation
        $string = bin2hex($string);

        return $string;
    }

    function get_initialization_vector()
    {
        // See: http://php.net/manual/en/mcrypt.ciphers.php
        // MCRYPT_BLOWFISH selected as it appears to be one of the "universally"
        // supported ciphers supported by the mcrypt extension
        $initialization_vector_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_ECB);

        // See: http://php.net/manual/en/function.mcrypt-create-iv.php
        // The source can be MCRYPT_RAND (system random number generator), MCRYPT_DEV_RANDOM
        // (read data from /dev/random) and MCRYPT_DEV_URANDOM (read data from /dev/urandom).
        // Prior to 5.3.0, MCRYPT_RAND was the only one supported on Windows.
        $initialization_vector = mcrypt_create_iv($initialization_vector_size, MCRYPT_RAND);

        return $initialization_vector;
    }

    function get_random_string($character_set = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz', $minimum_length = 8, $maximum_length = 12)
    {
        if ($minimum_length > $maximum_length)
        {
            $length = mt_rand($maximum_length, $minimum_length);
        }
        else
        {
            $length = mt_rand($minimum_length, $maximum_length);
        }

        $random_string = '';
        for ($i = 0; $i < $length; $i++)
        {
            $random_string .= $character_set[(mt_rand(0, (strlen($character_set) - 1)))];
        }

        return $random_string;
    }

    function hex2bin($hexadecimal_data)
    {
        $binary_representation = '';

        for ($i = 0; $i < strlen($hexadecimal_data); $i += 2)
        {
            $binary_representation .= chr(hexdec($hexadecimal_data{$i} . $hexadecimal_data{($i + 1)}));
        }

        return $binary_representation;
    }

Why Encryption Doesn't Fit the Goal of Password Storage

Cryptographic hashing and encryption have two different purposes. Hashing is not reversible -- given a hash, you can't determine what made it except by testing to see if a source input matches the hash output. When used for passwords, this becomes a validation only question: "does this password (mixed with this salt) match?" With passwords, the goal is to prevent somebody with full access to the database from recovering passwords.

Encrypting passwords handles that goal poorly. To verify a password, one must have the encryption key. That means one is also able to determine the source password from looking at the encrypted version of the password. In the event of a compromise, having those encrypted fields and the key will result in the disclosure of all passwords regardless of how strong they were.

Good Resources

https://security.stackexchange.com/questions/211/how-to-securely-hash-passwords

The Rules of Crypto

You'll hear it a lot around here: don't roll your own. The problem is that the field has changed too much for "common sense" to really be something that's working on our favor. Encryption and password issues have evolved relatively fast on a human-life scale of time. Where not everybody is following that, a lot of varying ideas exist about how to solve these problems and many of them have been proven wrong.

If you're not current, the proper approach is to research the issue you're trying to solve rather than the method you're trying to solve it with.


Finding substrings within arrays I need to find the substrings within my array. If I have an array: ["abc", "abcd", "abcde", "xyz"], my method should return the array members: abc, abcd, abcde as each is a substring or a superstring of the other, but it should exclude "xyz" as it is not related to other strings in any way.

function find_substrings(arr) {
    var res = [];
    for (var i=0; i<arr.length; i++) {
        for (var j=0; j<arr.length; j++) {
            if (i !== j && (arr[i].indexOf(arr[j]) > -1 || arr[j].indexOf(arr[i]) > -1)) {
                res.push(arr[i]);
                break;
            }
        }
    }
    return res;
}
var arr = ["abc", "abcd", "abcde", "xyz"];
console.log(find_substrings(arr));

Here my code is of \$O(n^2)\$ complexity, as it's iterating twice over the complete array. Is there any optimal solution?

Interesting question,

find_substrings should really be findSubstrings
I dislike arr, and prefer list
If you sort the array by the length of strings, then you never have to check prior elements
If you kept a list of matches and non-matches, then you would never have to check matches again
I found the test case troubling, since you are looking for only 1 set, whereas the code will retrieve any number of sets
The complexity calculation becomes tricky here, I would think a worst case would be n*n/2, and best case n.

In the end, I came up with something like this:

function findSubstrings(list) {
  var out = [],
    match, rest, i, j;
  //Sort the array by the length of the string
  list.sort((a, b) => a.length - b.length);

  for (i = 0; i < list.length - 1; i++) {
    match = [];
    rest = [];
    for (j = i; j < list.length; j++) {
      if (list[j].indexOf(list[i]) > -1) {
        match.push(list[j]);
      } else {
        rest.push(list[j]);
      }
    }
    //Did we find a set?
    if (match.length > 1) {
      out = out.concat(match);
      list = rest;
      i = 0;
      //Shortcut, should we still bother looking?
      if (rest.length == 0 || rest.length == 1) {
        return out;
      }
    }
  }
  return out;
}
var testCase = ["abc", "abcd", "abcde", "xyz", "tom", "tomd"];
console.log(findSubstrings(testCase));


Remove and Add items from array in nested javascript object I have the following object at React state:

groups: [
    {
        id: 1,
        items: [
            {id: 1},
            {id: 2},
            {id: 3}
        ]
    },
    {
        id: 3,
        items: [
            {id: 1},
            {id: 3},
            {id: 4}
        ]
    }
]

I have written the following functions to add and remove items:

  removeItem(groupId, itemId) {
    // make a copy
    let {groups} = this.state

    const gIndex= groups.findIndex(g=> g.id == groupId);
    let items = groups[gIndex].items
    groups[gIndex].items = items.filter((i) => i.id !== itemId)

    this.setState({groups})
  }

  addItem(groupId, itemId) {
    // make a copy
    let {groups} = this.state

    const gIndex = groups.findIndex(g => g.id == groupId);
    let items = groups[gIndex].items
    groups[gIndex].items = items.concat({id: itemId})

    this.setState({groups})
  }

Is there any way to write it cleaner?

For the remove function, how about using a map and filter instead:

removeItem (groupId, itemId){
    let {groups} = this.state;
    groups = groups.map((group) => {
        if(group.id === groupId){
           group.item = group.item.filter(item => item.id !== itemId);
        }
        return group;
    });
    this.setState({groups});
}


Class to simplify skipping some actions on the first loop iteration Often I have to skip some actions on the first (or last) loop iteration. Now I need to append some arguments to a URL and separate the arguments by the & character.

For example:

http://api.openweathermap.org/data/2.5/weather?q=London&mode=xml&cnt=7

To do so, the code looks like this:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    boolean firstIteration = true;
    StringBuilder urlBuilder = new StringBuilder(url);
    for (NameValuePair argument : arguments) {
        if (firstIteration) {
            firstIteration = false;
        } else {
            urlBuilder.append("&");
        }
        urlBuilder.append(argument.getName());
        urlBuilder.append("=");
        urlBuilder.append(argument.getValue());
    }
    return urlBuilder.toString();
}

But it's very annoying to write and a little difficult to read such code. And it's easy to make a mistake in a loop body like this.

I wrote the next class, but I need your help to name the class and its methods properly (I'm pretty bad in English). And please, criticize my code severely.

// I know that class name must be a noun.
// But I can't think of a good name.
// And it seems to me that a comparison with the blank cartridge is a good one.
public abstract class FirstCartridgeIsBlank implements Runnable {

    private boolean firstCartridge;

    public FirstCartridgeIsBlank() {
        firstCartridge = true;
    }

    public final void run() {
        if (firstCartridge) {
            firstCartridge = false;
            blankShot();
        } else {
            shot();
        }
    }

    protected void blankShot() {
        // the inheritors can override this method if they need
    }

    protected abstract void shot();

}

Now the method prepareUrl() looks like this:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    final StringBuilder urlBuilder = new StringBuilder(url);
    FirstCartridgeIsBlank appendAmpersand = new FirstCartridgeIsBlank() {
        @Override
        protected void shot() {
            urlBuilder.append("&");
        }
    };
    for (NameValuePair argument : arguments) {
        appendAmpersand.run();
        urlBuilder.append(argument.getName());
        urlBuilder.append("=");
        urlBuilder.append(argument.getValue());
    }
    return urlBuilder.toString();
}

I can't use the String.join() method from Java 8 because it's for the Android app (i.e. Java 6).

Don't try to over-generalize early. This is a situation I've run into quite a few times for strings, but never anywhere else, so I would recommend you start with a string-specific solution, and only try to come up with something more general if it ever becomes necessary.

You mention String.join- is there any situation where you face this problem where String.join wouldn't solve your problem? It's quite possible that the problem you actually need to solve isn't "skipping some actions on the first loop iteration", it's "cleanly joining strings with a separator". If that's the case, then why not just reimplement String.join yourself, rather than creating your own very different way of achieving the same thing?

If you do need to generalize, then your solution could work, but its main problem is that now you're creating a class whose only purpose is to run a chunk of some arbitrary (possible private!) procedure, just because that chunk happened to follow a particular structure. If Java 6 supported functional programming, then a single class which was passed functions would have been acceptable, but the need to create a new class that inherits from FirstCartridgeIsBlank every time you want to use it seems like a design headache to me.

So I think instead of trying to create a class for this, it'd probably just be simpler to do it by extracting methods:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    boolean firstIteration = true;
    StringBuilder urlBuilder = new StringBuilder(url);
    for (NameValuePair argument : arguments) {
        String argumentString = getQueryArgument(argument.getName(), argument.getValue(), firstIteration);
        firstIteration = false;
    }

    return urlBuilder.toString();
}

private static String getQueryArgument(String name, String value, Boolean includeSeparator) {
    //...
}


Calculate the average score of groups and each student, C I have a file containing information about the session of students. The structure of this file is: group number, student name, exams, and tests.

debt() function find and print the names of underachieving students.
student_score() calculate the average score of groups
group_score() calculate the average score of each student.

So, this is my program and maybe I can make something better?

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <wchar.h>
#include <locale.h>

#define N 50

void debt();
void student_score();
void group_score();

struct STUDENTS {
  int group;
  char name[100];
  char test[15];
  int exam;
};

struct STUDENTS student_list[N];

int main() {
  FILE *file;
  int choice;
  char buffer[200];
  int i = 0;

  file = fopen("students.txt", "r");

  if (file == NULL) {
    puts("students.txt не найден");
    return 1;
  }

  while (fgets(buffer, sizeof buffer, file) != NULL) {
    sscanf(strdup(buffer), "%d %[^0-9] %d %s", &student_list[i].group, student_list[i].name, &student_list[i].exam, student_list[i].test);
    i++;
  }
  fclose(file);
  int student_list_size = i;

  do {
    puts("\nВыберите номер пункта меню:");
    puts("1 - Cтуденты, имеющие задолженности");
    puts("2 – Средний балл каждого студента");
    puts("3 – Средний балл всей группы");
    puts("4 - Выход\n");

    scanf("%d", &choice);

    switch (choice) {
      case 1:
        debt(student_list_size);
        break;
      case 2:
        student_score(student_list_size);
        break;
      case 3:
        group_score(student_list_size);
        break;
      case 4:
        break;
      default:
        puts("\nНужно вводить номер пункта от 1 до 4");
        break;
    }
  } while (choice != 4);

  return 0;
}

void debt(int student_list_size) {
  int counter_debt;
  wchar_t test_wchar[15];
  wchar_t * pch;

  setlocale(LC_ALL, "");

  for (int i = 0; i < student_list_size; i++) {
    counter_debt = 0;
    mbstowcs(test_wchar, student_list[i].test, 15);

    pch = wcschr(test_wchar, L'н');
    while (pch != NULL) {
      counter_debt++;
      pch = wcschr(pch + 1, L'н');
    }

    if (counter_debt > 0) {
      printf("%d %s: %d\n", student_list[i].group, student_list[i].name, counter_debt);
    }
  }
}

void student_score(int student_list_size) {
  int exam;
  int average;
  int digit;
  int counter_digits;

  for (int i = 0; i < student_list_size; i++) {
    average = 0;
    digit = 0;
    counter_digits = 0;
    exam = student_list[i].exam;

    while (exam) {
      digit = exam % 10;
      average += digit;
      counter_digits++;
      exam = exam / 10;
    }

    printf("%d %s: %.1f\n", student_list[i].group, student_list[i].name, (double)average / counter_digits);
  }
}

void group_score(int student_list_size) {
  int exam;

  struct GROUP_SCORE {
    int group_score;
    int students_counter;
  };
  struct GROUP_SCORE group_list[5000] = {0};

  for (int i = 0; i < student_list_size; i++) {
    group_list[ student_list[i].group ].students_counter += 1;
    exam = student_list[i].exam;
    while (exam) {
      group_list[ student_list[i].group ].group_score += exam % 10;
      exam = exam / 10;
    }
  }

  for (int j = 0; j < 5000; j++) {
    if (group_list[j].group_score > 0) {
      printf("%i: %.1f\n", j, (double)group_list[j].group_score / group_list[j].students_counter);
    }
  }
}

students.txt:

4272 Галкин Г. А. 5445 ззззз
4273 Константинопольский А. А. 4333 знзнз
4273 Курочкин А. А. 3433 знзнн
4272 Козлов И. И. 4443 ззззн

Completely prototype functions: debt() student_score() group_score();

// void debt();
void debt(int student_list_size);

Avoid naked magic numbers.  Use #define or a constant to self document various numbers.

#define TEST_N (15)
struct STUDENTS {
  ...
  char test[TEST_N];
};

void debt(int student_list_size) {
  // wchar_t test_wchar[15];
  wchar_t test_wchar[TEST_N];
  ...
  // mbstowcs(test_wchar, student_list[i].test, 15);
  mbstowcs(test_wchar, student_list[i].test, TEST_N);

Use size_t for array index types and the result of sizeof.  int may not be the right size to use as an index.  (Also i in main())

 // void student_score(int student_list_size) {
 void student_score(size_t student_list_size) {
   ...
   // for (int i = 0; i < student_list_size; i++) {
   for (size_t i = 0; i < student_list_size; i++) {

Localize variables.  This approaches a style issue, but declaring variables close to their use has value.

void student_score(int student_list_size) {
  // int exam;
  // int average;
  // int digit;
  // int counter_digits;

  for (int i = 0; i < student_list_size; i++) {
    int exam;
    int average;
    int digit;
    int counter_digits;

The strdup() serves no purpose.  Eliminate it.

// sscanf(strdup(buffer), "%d ...
sscanf(buffer, "%d ...

The return value of sscanf() should be checked and string widths limited.  Note: student_list[i].name will have a ' ' at the end.

// sscanf(buffer, "%d %[^0-9] %d %s", &student_list[i].group, ...
if (4 != sscanf(buffer, "%d %99[^0-9] %d %14s", &student_list[i].group, ...) {
   Handle_Format_Error();

Use valid main() signature.

int main() {
int main(void) {

Conversions char to/from wchar_t may need different sizes.
The extra '\n' in puts("4 - Выход\n"); looks wrong.  Guess I am not a fan of puts().  Why not use 1 function call?

fputs("\nВыберите номер пункта меню:\n"
      "1 - Cтуденты, имеющие задолженности\n"
      "2 – Средний балл каждого студента\n"
      "3 – Средний балл всей группы\n"
      "4 - Выход\n"
      "\n", stdout);


Card shuffling with Fisher-Yates method The code below takes a specified number of card decks, and shuffles them according to the Fisher-Yates method. Does this implementation have any bias?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace randomShuffle
{
    class Program
    {
        static void Main(string[] args)
        {
            List<int> deck = Cards.startDeck(1);
        }
    }

    class Cards
    {
        public static List<int> startDeck(int numDecks)
        {
            List<int> initial = new List<int>();
            List<int> shuffled = new List<int>();

            //set up list of all cards in order
            while (numDecks > 0)
            {
                for (int i = 2; i <= 14; i++) //2-ace(14)
                {
                    for (int j = 0; j < 4; j++)  //each card in deck 4 times
                    {
                        initial.Add(i);
                    }
                }

                numDecks--;
            }

            //Fisher-Yates method to shuffle
            Random r = new Random(DateTime.Now.Millisecond);
            int count = initial.Count;
            for (int i = 0; i < count; i++) //go through entire unshuffled deck
            {
                //get random number from 0 to new range of unshuffled deck
                int randomDraw = r.Next(0, initial.Count);

                //take whatever is drawn and insert at beginning of shuffled list
                shuffled.Insert(0, initial[randomDraw]);

                //replace the card drawn with the end card
                initial[randomDraw] = initial.Last();

                //remove the end card from initia deck
                initial.RemoveAt(initial.Count - 1);
            }

            return shuffled;
        }
    }
}

Comments:

about all of your comments add close to no value to your code.

//get random number from 0 to new range of unshuffled deck
int randomDraw = r.Next(0, initial.Count);

Your comment just writes out, what your code does in the next line. This is unneccesary noise and you should not do it. The probably only acceptable comment is this one:

for (int i = 2; i <= 14; i++) //2-ace(14)

This exactly explains why you start on 2 and end with 14, the rest can be removed.

But as @Marc-Andre pointed out correctly, this comment also becomes pure noise, as soon as you extract these magic numbers to named constants:

const int LOWEST_CARD = 2;
const int HIGHEST_CARD = 14;
for (int i = LOWEST_CARD; i <= HIGHEST_CARD; i++)

Extracting methods:

Why not extract your whole shuffle algorithm to a method? This makes the code in your startDeck less cluttered. Example:

public static List<int> startDeck(int numDecks)
{
    List<int> initial = new List<int>();

    prepareDeck();

    return shuffled(initial);
}

You now exactly know, what your startDeck does.

Naming:

Your class Cards. this is definitely not nice, but that name is not OK. What does your class do? Name it after that. Try to keep your classes in SRP, they only should do one thing and nothing more.

Deck might have been better depending on what it does apart from what you posted.
Alternatively use Shuffler, Helper or similar.

Algorithm:

The english wikipedia article on the Fischer-Yates algorithm gives pretty nice pseudocode and tells us, that the algorithm is an in-place algorithm.
This means you will not need a second list to put your shuffled cards in.

//To shuffle an array a of n elements (indices 0..n-1):
for i from n − 1 downto 1 do
      j ← random integer with 0 ≤ j ≤ i
      exchange a[j] and a[i]

this means, if you were strictly following the algorithm your code should look (something) like this:

int temp, randomNumber;
int count = initial.Count;
for (int index = count -1; index > 0; index--){
   randomNumber = r.Next(0, index+1);
   temp = initial [index];
   initial[index] = initial[randomNumber];
   initial[randomNumber] = temp;
}
return initial;


Perl recursive copy and move functions I know a module like this already exists on CPAN, but I wanted to write my own simplified version that accepts wildcards in the input.

I don't write many Perl modules so I figured I would post the code here and see if anyone could give me advice on how to make this code more efficient or user friendly.

I also included some POD documentation so you can use perldoc for usage information.

Please let me know what you think and leave some feedback on how this code could be improved!

package File::Copy::Recursive;

# Use statements
use File::Basename;
use File::Copy;
use File::Path qw(make_path remove_tree);
use strict;
use warnings;

# Link the version number with CVS
our ($VERSION) = (q$Revision: 1.1 $ =~ m{([0-9.]+)});

# Exporter
require Exporter;
our @ISA = qw(
    Exporter
);

# Declare exportable routines
our @EXPORT_OK = qw(
    recursive_copy rcopy
    recursive_move rmove
);

# Aliases to subroutines
sub rcopy { recursive_copy(@_); }
sub rmove { recursive_move(@_); }

# Recursive copy function
sub recursive_copy {
    my ($source, $destination) = @_;
    for my $path (glob $source) {
        if (-d $path) {
            make_path($destination.'/'.basename($path));
            recursive_copy($path.'/*', $destination.'/'.basename($path));
        }
        else {
            copy($path, $destination);
        }
    }
}

# Recursive move function
sub recursive_move {
    my ($source, $destination) = @_;
    for my $path (glob $source) {
        if (-d $path) {
            make_path($destination.'/'.basename($path));
            recursive_move($path.'/*', $destination.'/'.basename($path));
            remove_tree($path);
        }
        else {
            move($path, $destination);
        }
    }
}

1;

__END__

=head1 NAME

File::Copy::Recursive - Recursive copy and move functions.

=head1 VERSION

$Revision: 1.1 $

=head1 SYNOPSIS

  use File::Copy::Recursive;

  # Recursively copy a directory
  recursive_copy('input', 'output');

  # Recursively copy all contents of a directory
  recursive_copy('input/*', 'output');

  # Recursively move a directory
  recursive_move('input', 'output');

  # Recursively move all contents of a directory
  recursive_move('input/*', 'output');

=head1 DESCRIPTION

File::Copy::Recursive is designed to provide simple functions to recursively copy and move files.
These functions will accept wildcards (*) in the input.

=head1 SUBROUTINES/METHODS

=over

=item C<recursive_copy> or C<rcopy>

Recursive copy function.

  # Recursively copy a directory
  recursive_copy('input', 'output');

  # Recursively copy all contents of a directory
  recursive_copy('input/*', 'output');

=item C<recursive_move> or <rmove>

Recursive move function.

  # Recursively move a directory
  recursive_move('input', 'output');

  # Recursively move all contents of a directory
  recursive_move('input/*', 'output');

=back

You've got a security vulnerability here just waiting to be exploited based on the fact you use the same interface for "The human" and "The Filesystem".

All you need is for somebody to create a file with:

open my $fh, '>', 'path/*'

And that will get glob-expansion during traversal.

Which means you DON'T want to use glob at a level other than the user-facing function call for the sake of convenience.

The user facing function should probably use use Text::Glob::glob_to_regex so you can generate a "match rule" and re-use it for subsequent operations,
instead of relying on glob to return a list of files.

When the function calls itself, it should use opendir + readdir which is not subject to glob expansion.

Your code also assumes a lot about OS File Path semantics.

So for Filesystem manipulation, I would probably suggest lots of File::Spec or Path::Tiny, as they're far more battle tested against Path handling, and the later provides a few helper functions that will make tree traversal and iteration much more obvious. ( And importantly, much safer )

As for the Exporter call, this style is more recommended:

use Exporter 5.57 qw( import );

This simply creates the import sub on your package instead of fussing with @ISA and thus eliminates the need for Exporter to be part of your inheritance tree.

If you want to support versions of Exporter older that 5.57, this is equally effective and can be expected to work on a stock 5.6 perl:

use Exporter ();
*import = \&Exporter::import;

Example

The following code constructs a diabolical directory that will result in graph traversal happening not once, but several times.

#!/usr/bin/env perl
use strict;
use warnings;

## Creating an example "Evil" directory to traverse.

use Test::TempDir::Tiny qw( tempdir );
use Path::Tiny qw( path );

my $dir = tempdir();

# Problem directories
path( $dir, "evil/*" )->mkpath;
path( $dir, "evil/*/starfile" )->touch;
path( $dir, "evil/**" )->mkpath;
path( $dir, "evil/**/doublestarfile" )->touch;

# Normal Files
path( $dir, "evil/child/deep" )->mkpath;

path( $dir, "evil/child" )->mkpath;
path( $dir, "evil/child/a" )->touch;
path( $dir, "evil/child/b" )->touch;
path( $dir, "evil/child/deep/a" )->touch;
path( $dir, "evil/child/deep/b" )->touch;

## Evil directory now constructed, emulated simplified recursion
## using glob

our $DEPTH = 0;
our $PAD   = '';

sub list_files_recursively {
  my ( $path ) = @_;
  STDERR->print("${PAD}?: $path\n");
  local $DEPTH = $DEPTH + 1;
  local $PAD   = ' ' x $DEPTH;
  my @out;
  ###### DONT DO THIS ######
  for my $leaf ( glob $path ) {
  ###### DONT DO THIS ######
    if ( -d $leaf ) {
      STDERR->print("${PAD}d: $leaf\n");
      push @out, list_files_recursively( $leaf . '/*' );
    }
    else {
      STDERR->print("${PAD}f: $leaf\n");
      push @out, $leaf;
    }
  }
  return @out;
}

for my $entry ( list_files_recursively( "$dir/*" ) ) {
  print $entry . "\n";
}

print "Expected layout:\n";
system("find",$dir,"-type", "f" );

Executing this code will reveal list_files_recursively "visits" the same files multiple times.

Full output here omitted for brevity

The most important part is to note what happens when it tries to list the children of the directory called **

  d: /tmp/7LYkL6zdrB/test_pl/default_1/evil/**
  ?: /tmp/7LYkL6zdrB/test_pl/default_1/evil/**/*
   f: /tmp/7LYkL6zdrB/test_pl/default_1/evil/**/doublestarfile
   f: /tmp/7LYkL6zdrB/test_pl/default_1/evil/*/starfile
   f: /tmp/7LYkL6zdrB/test_pl/default_1/evil/child/a
   f: /tmp/7LYkL6zdrB/test_pl/default_1/evil/child/b
   d: /tmp/7LYkL6zdrB/test_pl/default_1/evil/child/deep

Yes, that's thinking "Hey, it looks like 'child/deep' is a child of **".

Which it really really isn't.

And there are probably plenty more glob rules that can be exploited here, * is just the most dangerous.

For instance, glob interprets spaces as token delimiters, so glob "a b"  -> 2 entries, not 1.


Combining multiple jQuery animations? I have a couple of jQuery animations running in a sequence after certain resources are loaded:

$(".loading").delay(2600).fadeOut({
    useTranslate3d: true,
});

$(".zip").delay(1900).slideDown({
    useTranslate3d: true,
});

$("#zip").delay(2800).slideDown({
    useTranslate3d: true,
});

Is there a better way to combine all of these animations? It feels superfluous having to write them all out like this...

You can use the same options object for all three animations.

var opts = {
    useTranslate3d: true,
}
$(".loading").delay(2600).fadeOut(opts);
$(".zip").delay(1900).slideDown(opts);
$("#zip").delay(2800).slideDown(opts);

Having .zip and #zip pointing to different elements might also be a bit confusing - but that depends on your DOM structure.


PHP program which extracts attribute lists from custom tag PHP program which extracts attribute list from the custom tag. It also strips out the single and double quote if it exists around attribute value.

<?php

function parseAnswerTag($tag)
{
    // input => <answer scale="10" points="7">
    // ouput => ["scale"=>10, "points"=>7]
    // get the answer tag and parse out the attributes of answer tag
    $returningObj = array();
    //get the splitted string as an array
    $arr = splitByDelimiter(array('delimiter' => " ", 'string'=> $tag));
    // sliced the array to just have attributes otherwise will have answer element too
    foreach (array_slice($arr, 1) as $key => $value)
    {
        $arr[$key] = splitByDelimiter(array('delimiter' => "=", 'string'=> $value));
        if(in_array('scale', $arr[$key]) or in_array('points', $arr[$key]))
        {
            // check to see if single or double quote exist in value of key
            if(preg_match("/['\"]/",$arr[$key][1]))
            {
                // makes a call to strip out the quotes
                $returningObj[$arr[$key][0]] = stripQuotes($arr[$key][1]);
            }
        }
    }
    // returns the associate key of attribute in the tag
    return $returningObj;
}

function stripQuotes($str)
{
    // input => "10"
    // output => 10
    // gets attribute with value and returns unquote string
    var_dump(strpos($str, "'") === FALSE);
    $start_point = 0;
    $end_point = 0;
    if(strpos($str, "'") !== FALSE)
    {
        $start_point = strpos($str, "'");
        $end_point = strpos($str, "'", $start_point+1);
    }
    elseif (strpos($str, '"') !== FALSE)
    {
        $start_point = strpos($str, '"');
        $end_point = strpos($str, '"', $start_point+1);
    }
    return substr($str, $start_point+1, $end_point - ($start_point+1));
}

function splitByDelimiter($input)
{
    //gets a associate array with delimiter and string {text value to be splitted}, split them by passed delimiter and return them
    // $input['delimiter'] => value splitted by and $input['string'] => value to be splitted
    return explode($input['delimiter'], $input['string']);
}

Can you please give me some constructive suggestions so that I can improve?

Here are some things I noticed:

The function splitByDelimiter() is no more than a wrapper around explode(). Why not use explode() directly?
The function stripQuotes($str) can be a lot shorter: trim($str," '\"");. Note: You should never use ' or " inside attributes, use their HTML equivalents.
Inside parseAnswerTag() you use a foreach loop. Inside it you assign values back into $arr, but under the wrong keys because array_slice() will have generated new numeric keys.
I don't understand why you select for scale and point?

I'm sorry but I cannot help you with other details, because I'm unsure about the origin of the HTML, and what you exactly are trying to accomplish. I did make a shorter version of your function:

function parseAnswerTag($tag)
{
    $inside = trim($tag,'<> ');
    $input  = explode(' ',$inside);
    array_shift($input);
    foreach ($input as $attr)
    {
        $parts = explode('=',$attr);
        $name  = trim($parts[0]);
        $value = trim($parts[1]," '\"");
        $output[$name] = $value;
    }
    return $output;
}

echo '<pre>';
print_r(parseAnswerTag('<answer scale="10" points="7">'));
echo '</pre>';

So trimming can be very useful.

Keep in mind: Algorithms like these are almost always slightly flawed. A simple thing like a double space could trip it up. It's very hard to get it completely right. Especially if you make it complicated or use regular expressions.

I also have to point out that PHP has a build in DOM to deal with HTML code. You can load HTML into it, and manipulate it there. See:

http://php.net/manual/en/class.domdocument.php

In more complex cases this is the preferred way to deal with HTML.


Method to count all comments in single external C# file I recently had an interview question:

  Write a method that counts all comments in a single external file.

It was a timed question and I wanted to know if this is the best way to accomplish the task. I'm also open to improvements, advice, etc.

(I didn't pass the interview.)

class Program
{
    static void Main(string[] args)
    {
        List<string> FileLines = new List<string>();
        const string Filename = @"file.txt";

        readInFile(Filename, FileLines);

        CountComments(FileLines);
    }

    public static void CountComments(List<string> value)
    {
        int finalCount = 0;
        List<string> something = value;
        bool inQuotes = false;

        for (int x = 0; x < something.Count; x++)
        {
            if (something[x] != @"""")
            {
                inQuotes = !inQuotes;
            }
            for (int y = 0; y < something[x].Length; y++)
            {
                if (something[x][y] == '/' && something[x][y + 1] == '/')
                {
                    finalCount++;
                    break;
                }
                else if (something[x][y] == '/' && something[x][y + 1] == '*')
                {
                    finalCount++;
                    break;
                }
            }
        }
        Console.WriteLine("Total number of comments is " + finalCount);

    }

    public static void readInFile(string Filename, List<string> FileLines)
    {
        using (StreamReader r = new StreamReader(Filename))
        {
            string Line;

            while ((Line = r.ReadLine()) != null)
            {
                FileLines.Add(Line);
            }
        }
    }
}

Naming
The first thing I notice, is inconsistency with naming.
Locals

FileLines
Filename
finalCount
something
inQuotes
x
y
r
Line

Locals should be camelCase, and should have meaningful names. x, y, r and something aren't meaningful names. Filename should be fileName.
Parameters

args
value
Filename
FileLines

Parameters are like locals - they should be camelCase. Filename should be fileName, and FileLines should be fileLines.
Methods

Main
CountComments
readInFile

Method/member names should be PascalCase. readInFile should be ReadInFile - and that's not exactly a great name either. You're reading the file into a List<string> ...that's passed as a parameter. Not the most intuitive approach.

Accessibility
Methods CountComments and readInFile have no business being public.

Constant vs Read-Only
This variable has no business being a const:

const string Filename = @"file.txt";

It should be an instance-level private static readonly string field, because a const should be strictly for something that has zero chance of ever changing in a future version. A file path is definitely something that can change, declaring it as a const is a semantic mistake.

Approach
readInFile should have a return type instead of having the side-effect of adding items to a List<string> parameter. While legal, a better way to convey that a method will have side-effects on a parameter, is to ask for an out parameter, or to pass the list by reference using the ref keyword. Your method takes its output through its input channel, and that doesn't feel right at all.
I would have gone with File.ReadAllLines instead, which returns a string[] array where each element is a line in the specified file, or better (given C# 4+), File.ReadLines, which returns an IEnumerable<string> instead: this means the whole readInFile method could have been inlined, and the FileName just specified as a hard-coded parameter value (or read from the args command-line arguments, if you wanted to get fancy).
As for the actual commend-finding logic...

if (something[x][y] == '/' && something[x][y + 1] == '/')
{
    finalCount++;
    break;
}
else if (something[x][y] == '/' && something[x][y + 1] == '*')
{
    finalCount++;
    break;
}

Can you spot the smell? Why do you need two branches, if both are going to do exactly the same thing?
I would have expected CountComments to do just that: count the comments. Yours is breaking SRP by also performing output - the method should've had an int return type, and just returned the result, leaving it up to the caller to decide what to do with it.
This one puzzles me:

List<string> something = value;

Why not work off value? Why introduce a new meaningless identifier (something? really?) to make a copy of another meaningless identifier (value would already be better as values, but lines or even content would have been sooooo much better!).
I like that you stop iterating characters in a line after you've found a comment.
But instead of nested loops, I would have written a function that takes a string and returns a bool, to encapsulate the logic that basically says "is there a comment anywhere in that string?" - assuming C# 3.5+, the whole method could have looked like this:
return lines.Count(HasComment);

If you were constrained to 2.0, you would've had to perform the loop explicitly - still (assuming a string[] lines parameter):
int result = 0;
for (int line = 0; line < lines.Length; line++)
{
    if (HasComment(lines[line]))
    {
        result++;
    }
}
return result;

And then depending on the time remaining you could have refined the HasComment logic, for example to skip lines that are inside a multiline comment.


Shortening words within a string The function receives a string containing a sentence. Its goal is to shorten every word in it to a length of 4 characters until the string is shorter than 38 characters and return.

The 38 characters max length is mandatory and must be reached with the least amount of deletion possible.

Here is what I've done:

# reduces each word to a lenght of 4
def shorten_words(string):
    i = 0
    j = 0
    while(len(string) > 38 and i < len(string)):
         array = list(string)
         if(array[i] != ' ' and array[i] != '-'):
             j += 1
         else:
             j = 0
         if(j > 4):
             array[i] = ''
             i -= 1
         i += 1
         string = ''.join(array)
    return(string)

I feel like the list/join method is inefficient and would like to know how I could improve this function

Some suggestions:

Naming is really important. string, array, i and j are not descriptive. After reading the entire function I think they could be renamed sentence, words, string_index and word_length.
There's no point in adding empty strings to the array - they aren't printed anyway.
What is the significance of 38? If it's not significant it should be removed, if it is it should be named something like max_result_length.
In Python return is a simple statement, which means its argument should not be put in parentheses.


Simple shop database site I'm learning PHP, HTML and SQL - and wondered if anyone could take a look at this code and give me some feedback?

<!DOCTYPE html>

    <link rel="stylesheet" href="styles.css">
    <title>Store product information</title>

    <?php

    $host = "localhost";
    $database = "shopDB";
    $username = "root";
    $password = "";

    $conn = new mysqli($host, $username, $password);
    $db_handle = $conn->select_db($database);

    if ($db_handle)
    {
        if ($_SERVER['REQUEST_METHOD'] == 'POST')
        {
            // Upload to server
            $name = mysqli_real_escape_string($conn, $_POST['name']);
            $quantity = mysqli_real_escape_string($conn, $_POST['quantity']);
            $price = mysqli_real_escape_string($conn, $_POST['price']);
            $description = mysqli_real_escape_string($conn, $_POST['description']);

            $insert_sql = "INSERT INTO products (name, quantity, price, description) VALUES ('$name', '$quantity', '$price', '$description')";
            $insert_result = $conn->query($insert_sql);

            if ($insert_result)
            {
                // Uploaded successfully
            } else {
                print mysqli_error($conn);
            }
        }

        $products_sql = "SELECT
                           products.name,
                           products.quantity,
                           products.price,
                           products.description
                         FROM products";
        $products_result = $conn->query($products_sql);

        if ($products_result)
        {
            print "
";

            print "
";
            print "
";
            print "Name	";
            print "Quantity	";
            print "Price	";
            print "Description	";
            print "
";
            print "
";

            print "
";
            while ($db_field = mysqli_fetch_assoc($products_result))
            {
                print "
";
                print "" . $db_field['name'] . "	";

                if ($db_field['quantity'] == '0') {
                    print "" . $db_field['quantity'] . "	";
                } else {
                    print "" . $db_field['quantity'] . "	";
                }

                print "" . $db_field['price'] . "	";
                print "" . $db_field['description'] . "	";
                print "
";
            }
            print "
";
            print "
";
        }
    }

    ?>

Insert items

    <form action="index.php" method="post">

                Name
                Quantity
                Price
                Description

                <input name="name" type="text"></input>
                <input name="quantity" type="text"></input>
                <input name="price" type="text"></input>
                <input name="description" type="text"></input>

    <input id="submit_button" type="submit" value="Register item">
    </form>

Database structure:

Security

XSS

You might want to consider HTML encoding the database values such as product name, description, price, etc.

It may or may not be needed currently, but generally, you might want more than one type of user in the future. You may have a site admin, a sales manager, data entry people who add new products, and so on.

If you do not encode these values, less privileged users could escalate their privileges via XSS.

CSRF

You also do not have any CSRF protection. This means that an attacker can add products if a logged in user with the right to add products visits an attacker-controlled website.

As you are also open to XSS, an attacker can gain XSS via adding products.

SQL Injection

Escaping is really not the preferred way to defend against SQL Injection. It's certainly a lot better than nothing, and if used correctly will be secure, but in practice, prepared statements result in better and more secure code and are really the only correct way to handle SQL Injections.

Structure

Your code is generally quite readable, but it would be even more readable if you would separate code into classes and functions. This would also make adding new functionality a lot easier

For later exercises, you might want to look into MVC. But even for a small example script, you might want to at least separate the database connection , so in case the credentials change, you do not have to change them all over the place, but just in one configuration file.

Functions would help make your code even more readable. You might consider functions such as selectProductById($connection, $id), insertProduct($connection, $name, $description, $price, $quantity), showProducts($products), showProductForm etc. You might also consider extracting those methods in a separate Product class (or a Product class for the database interactions, and a ProductView class for the displaying functions).

Misc

why are all your inputs of type text? Quantity and Price should be numbers, allowing a user to enter anything makes for bad usability. In practice, description should also likely be a textarea.
having an empty if is bad for readability. Just negate the question and only have the else case.
regarding usability: Think about adding at least some kind of feedback after a successful add.


Thread-safe concurrent FIFO queue in C++ Is this the correct way to implement a thread-safe concurrent FIFO queue in C++?  It requires passing unsigned char* arrays of binary data.

Thread Safe Concurrent Queue

#include <queue>
#include <pthread.h>

class concurrent_queue
{

private:

    std::queue<unsigned char*> _queue_;
    pthread_mutex_t push_mutex;
    pthread_mutex_t pop_mutex;
    pthread_cond_t cond;

public:

    concurrent_queue()
    {
        pthread_mutex_init(&push_mutex, NULL);
        pthread_mutex_init(&pop_mutex, NULL);
        pthread_cond_init(&cond, NULL);
    }

    void push(unsigned char* data)
    {
        pthread_mutex_lock(&push_mutex);

        _queue_.push(data);

        pthread_cond_signal(&cond);
        pthread_mutex_unlock(&push_mutex);
    }

    void pop(unsigned char** popped_data)
    {
        pthread_mutex_lock(&pop_mutex);

        while (_queue_.empty() == true)
        {
            pthread_cond_wait(&cond, &pop_mutex);
        }

        *popped_data = _queue_.front();
        _queue_.pop();

        pthread_mutex_unlock(&pop_mutex);
    }
};

CONSUMER TEST:

void *consumer_thread(void *arguments)
{
    concurrent_queue *cq = static_cast<concurrent_queue*>(arguments);

    while (true)
    {
        unsigned char* data = NULL;

        cq->pop(&data);

        if (data != NULL)
        {
            // Eureka! Received from the other thread!!!
            // Delete it so memory keeps free.
            // NOTE: In the real scenario for which I need
            // this class, the data received are bitmap pixels
            // and at this point it would be processed

            delete[] data;
        }
    }

    return 0;
}

PRODUCER TEST:

void main()
{
    concurrent_queue cq;

    // Create the consumer
    pthread_t consumer;
    pthread_create(&consumer, NULL, consumer_thread, &cq));

    // Start producing
    while(true)
    {
        // Push data.
        // Expected behaviour: memory should never run out, as the
        // consumer should receive the data and delete it.
        // NOTE: test_data in the real purpose scenario for which I
        // need this class would hold bitmap pixels, so it's meant to
        // hold binary data and not a string

        unsigned char* test_data = new unsigned char [8192];
        cq.push(test_data);
    }

    return 0;
}

You are guarding the state of one variable

std::queue<unsigned char*> _queue_;

So you only have one mutex.

pthread_mutex_t push_mutex;
pthread_mutex_t pop_mutex;

If you have two then push and pop can modify queue at the same time.

Make sure your mutex locking is exception safe by using RAII. Note all operations were you do start operation stop should be done by using RAII. This is the most important concept in C++ look it up.

Don't use pointers in C++. They do not convey ownership symantics (which means you don't know who should delete it (or when it should be deleted)). In this case use std::string it solves all the problems.

I see calls pthread_XXX_init but not the equivalent destroy.

Because you have not defined them the default copy constructor and assignment operator will be created. Since your object contains resources (mutext and conditional) you probably don't want the default behavior. Either define them or disable them.

C++11 has a better set of threading constructs use those rather than the C pthread library.


Points and segments solution Task:

  Given a set of points on a line and a set of segments on a line. The
  goal is to compute, for each point, the number of segments that
  contain this point.

  Input Format

  The first line contains two non-negative integers \$s\$ and \$p\$ defining the number of segments and the number of points on a
  line, respectively. The next \$s\$ lines contain two integers \$a_i, b_i\$
  defining the \$i\$th segment \$[a_i, b_i]\$. The next line contains \$p\$
  integers defining points \$x_1, x_2, \dots, x_p\$.

  Constraints

  \$1 \le s, p \le 50000\$
  \$−10^8 \le a_i \le b_i ≤ 10^8\$ for all \$0 \le i \lt s\$
  \$−10^8 \le x_j \le 10^8\$ for all \$0 \le j \lt p\$

  Output Format

  Output \$p\$ non-negative integers \$k_0, k_1, \dots , k_{p−1}\$ where \$k_i\$ is the number of segments which contain \$x_i\$.

My solution

For each point \$x_i\$ consider a pair (\$x_i, p\$). For each segment \$[a_i, b_i]\$ consider two pairs: (\$ai, 'l'\$) and (\$b_i, r\$) (\$p, l, r)\$ stand for point, left, and right, respectively).

Example: given three points \$x_1 = 5, x_2 = 8, x_3 = 3\$ and two segments \$[a_1, b_1] = [4, 10], [a_2,b_2] = [2, 6]\$. We then create the following list:

\$(5, p), (8, p), (3, p), (4, l), (10, r), (2, l), (6, r)\$

And then we sort it:

\$(2, l), (3, p), (4, l), (5, p), (6, r), (8, p), (10, r)\$

Now, let's scan it from left to right. The first item indicates the beginning of a segment (as it has l). The next is a point and we know that it is covered by one segment. The next item (4,l) indicates that we have the second segment. The next item (5,p) is a point and it is covered by two segments. The next item indicates the end of some segment, and so on.

import sys

def fast_count_segments(starts, ends, points):
    cnt = [0] * len(points)
    segments_num = 0

    listpoints = [(x, 'l') for x in starts]
    listpoints += [(x, 'p') for x in points]
    listpoints += [(x, 'r') for x in ends]

    listpoints.sort(key=lambda x: x[0], reverse=False)

    for p in listpoints:
        if p[1] == 'l':
            segments_num += 1
        elif p[1] == 'r':
            segments_num -= 1
        else:
            cnt[points.index(p[0])] = segments_num

    return cnt

if __name__ == '__main__':
    input = sys.stdin.read()
    data = list(map(int, input.split()))
    n = data[0]
    m = data[1]
    starts = data[2:2 * n + 2:2]
    ends   = data[3:2 * n + 2:2]
    points = data[2 * n + 2:]
    cnt = fast_count_segments(starts, ends, points)
    for x in cnt:
        print(x, end=' ')

Sample

Input:

2 3
0 5
7 10
1 6 11

Output:

1 0 0

For large input, my code run very slow, so I really need some reviews on my solution and my code to help reduce the runtime.

else:
    cnt[points.index(p[0])] = segments_num

Looking up the index of every point here is slow. A quick fix would be to store counts in a dict and traverse it in the correct order afterwards; see how that performs.

cnt = {}
segments_num = 0

else:
    cnt[p[0]] = segments_num

return [cnt[x] for x in points]

It also looks like your solution isn’t quite correct, since the sort could put lefts, rights, and points at the same location in different orders. Since the correct order of left, point, right also happens to be in alphabetical order, you can do another minimal fix and replace this:

listpoints.sort(key=lambda x: x[0], reverse=False)

with this:

listpoints.sort()

That’s a bit of a hack, but for this sort of task, it seems standard to keep the code short (fast to write in a competition?) rather than particularly expressive.


Repetitive replacement for expressing fractions in LaTeX I am converting natural math expressions with fractions to Latex code.

What I have done seems to work on examples I've tested, but I'm not sure if the algorithm is safe. And maybe the string part could be improved.

Maybe a better approach could be replace the / from left to right, but how in my case target the next?

By a generic pattern that matches the 3 cases at once.
By selecting specific / and passing tests on it and remplacing it after.

I don't know how to make that.

Some examples

((a)/(b))/(c)   ->   \dfrac{\dfrac{a}{b}}{c}
(a)/((b)/(c))   ->   \dfrac{a}{\dfrac{b}{c}}
(a)/(b)/(c)/(d)    ->   \dfrac{\dfrac{\dfrac{a}{b}}{c}}{d}
((a)/(b))/((c)/(d)) ->    \dfrac{\dfrac{a}{b}}{\dfrac{c}{d}}

My try :

function ToFrac (s)
    while s:find ("/") ~= nil
    do

    -- Remplace : \dfrac{}{}/() -> \dfrac{\dfrac...}{}
    if ( s:find ( '\\dfrac%b{}%b{}/%b()' , j ) ~= nil )
    then
    x,y,num,den = s:find( '(\\dfrac%b{}%b{})/(%b())' )
    den = den:gsub( '.(.+).' , '%1' )
    s = s:gsub( '(\\dfrac%b{}%b{})/(%b())',
                            "\\dfrac{"..num.."}{"..den.."}" , 1 )
    end

    print ('### -- ', s)

    -- Remplace : ()/\dfrac{}{} -> \dfrac{()}{\dfrac...}
    if ( s:find ( '(%b()/\\dfrac%b{}%b{}' ) ~= nil )
    then
    x,y,num,den = s:find( '((%b())/(\\dfrac%b{}%b{})' )
    num = num:gsub( '.(.+).' , '%1' )
    s = s:gsub( '((%b())/()\\dfrac%b{}%b{})',
                            "\\dfrac{"..num.."}{"..den.."}" , 1 )
    end

    print ('### -- ', s)

    -- Remplace : ()/() -> \dfrac{}{}
    if ( s:find ( '%b()/%b()' , 1 ) ~= nil )
    then
        x,y,num,den = s:find( '(%b())/(%b())' )
        num = num:gsub( '.(.+).' , '%1' )
        den = den:gsub( '.(.+).' , '%1' )
        s = s:gsub( '(%b())/(%b())',
                        "\\dfrac{"..num.."}{"..den.."}" , 1 )
    end

    print ('### -- ', s)

    end -- while

    return (s)

end

s = "((a)/(b))/(c)"
print (s, ToFrac(s))

s = "(a)/((b)/(c))"
print (s, ToFrac(s))

s = "(a)/(b)/(c)/(d)"
print (s, ToFrac(s))

s = "((a)/(b))/((c)/(d))"
print (s, ToFrac(s))

You can use a recursive function with the pattern %b() as follows:

function ToFrac( sInput )
    local tStack = {}
    for sOperand in sInput:gmatch "%b()" do
        table.insert( tStack, sOperand:sub(2, -2) )
    end
    while true do
        if #tStack <= 1 then break end
        local sNumerator = table.remove( tStack, 1 )
        if sNumerator:find '/' then sNumerator = ToFrac( sNumerator ) end
        local sDenominator = table.remove( tStack, 1 )
        if sDenominator:find '/' then sDenominator = ToFrac( sDenominator ) end
        table.insert( tStack, 1, ([[\dfrac{%s}{%s}]]):format(sNumerator, sDenominator) )
    end
    return tStack[1]
end

Here, I'm storing the operands as nodes in a stack. I tested the above function for the inputs:

local s = {
    "((a)/(b))/(c)",
    "(a)/((b)/(c))",
    "(a)/((b)/(c))/(d)",
    "(a)/(b)/(c)/(d)",
    "((a)/(b))/((c)/(d))",
}

for k, v in pairs(s) do
    print( v, ToFrac(v) )
end

The final results were:

((a)/(b))/(c)   \dfrac{\dfrac{a}{b}}{c}
(a)/((b)/(c))   \dfrac{a}{\dfrac{b}{c}}
(a)/((b)/(c))/(d)   \dfrac{\dfrac{a}{\dfrac{b}{c}}}{d}
(a)/(b)/(c)/(d) \dfrac{\dfrac{\dfrac{a}{b}}{c}}{d}
((a)/(b))/((c)/(d)) \dfrac{\dfrac{a}{b}}{\dfrac{c}{d}}