Login with password_hash() I am learning from a book, which uses SHA1. I've changed the code to password_hash(), by checking online. Here I query only the password and verify it. I managed get it working, but I feel a bit paranoid about the code. Is the code proper or wrong?

At registration:

//at registration
$p=mysqli_real_escape_string($dbcon, trim($_POST['psword1']));
$hash =  password_hash($p,PASSWORD_DEFAULT);
//stores $hash in db.

At login:

<?php
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    //connect to db
    require ('mysqli_connect.php');
    //validate the email
    if (!empty($_POST['email'])){
       $e = mysqli_real_escape_string($dbcon, $_POST['email']);
       }else {
       $e = FALSE;
       echo '
 You forgot to enter your email.
';
    }
// validate the password
if (!empty($_POST['psword'])){
      $p=mysqli_real_escape_string($dbcon,$_POST['psword']);
   } else {
       $p = FALSE;
       echo '
You forgot to enter your password.
';
     }
      if (!empty($e && $p )){
        //get email info
             $sql = "SELECT  psword FROM users WHERE email = '$e'  LIMIT 1";
             $query = mysqli_query($dbcon,$sql);
             $row = mysqli_fetch_row($query);
             $dbpassword=$row[0];

        //run if email and password got value
        if (password_verify($p,$dbpassword)){
          // Retrieve the user_id, first_name
          $q = "SELECT user_id, fname, user_level,lname,registration_date FROM users WHERE (email='$e')";
          // Run the query and assign it to the variable $result
          $result = mysqli_query ($dbcon, $q);
          // Count the number of rows that match the email/password combination
          if (@mysqli_num_rows($result) == 1) {//if one database row (record) matches
            // Start the session, fetch the record and insert the three values in an array
            session_start();
            $_SESSION = mysqli_fetch_array ($result, MYSQLI_ASSOC);
            // Ensure that the user level is an integer.
            $_SESSION['user_level'] = (int) $_SESSION['user_level'];
            // Use a ternary operation to set the URL
            $url = ($_SESSION['user_level'] === 1) ? 'index.php' : 'members.php';
            header('Location: ' . $url); // Make the browser load either the members’ or the admin page
            exit(); // Cancel the rest of the script
            mysqli_free_result($result);
            mysqli_close($dbcon);
        } else { // No match was made.
            echo '
The e-mail address and password entered do not match our records
Perhaps you need to register, just click the Register button on the header menu
';
          }
          } else { // If there was a problem.
            echo '
Please try again.
';
            }

   }
mysqli_close($dbcon);
}
?>

PLEASE do not do anything to the password prior to hashing it. Do not escape it. Do not trim() it. Just pass it directly to password_hash.

Secondly, you never fetch the hash from the database. That looks weird that you have a magic $dbpassword variable...

Third: Please use prepared statements. Escaping can be secure, but prepared statements are so much easier to verify and work with.

Fourth: Don't close the connection all over the place like that. Let it get closed for you by the script end, that way you don't need to reconnect multiple times in the same script.

Fifth: Please use consistent indentation. It will make your life a LOT easier.


Rot13 Unix Filter in C I'm trying to brush up on some C basics (it's been ten years since I've programmed in C) and am working on a simple Rot13 program as a Unix filter. Any suggestions on how to improve it would be appreciated. Specifically, I'm curious if the call to fclose at the end of main is necessary or will have bad potential effects since it is possible for stdin to be passed as an argument.

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

// input_from_args: returns a FILE* corresponding to the input
// source specified by the user. If no file was specified,
// returns stdin.

FILE* input_from_args(int argc, const char *argv[])
{
    if (argc == 1) {
        return stdin;
    } else {
        return fopen(argv[1], "r");
    }
}

// rot13: returns the rot13 encoding of the specified character

int rot13(int c)
{
    if (('a' <= c && c <= 'm') || ('A' <= c && c <= 'M')) {
        return c + 13;
    } else if (('n' <= c && c <= 'z') || ('N' <= c && c <= 'Z')) {
        return c - 13;
    } else {
        return c;
    }
}

// convert_all: converts all of the characters in src using the
// rot13 encoding writing the results to dest

void convert_all(FILE *src, FILE *dest)
{
    int c;

    while ((c = fgetc(src)) != EOF) {
        fprintf(dest, "%c", rot13(c));
    }
}

int main(int argc, const char *argv[])
{
    FILE *src = input_from_args(argc, argv);
    FILE *dest = stdout;

    if (src == NULL) {
        fprintf(stderr, "%s: unable to open %s\n", argv[0], argv[1]);
        exit(EXIT_FAILURE);
    }

    convert_all(src, dest);

    fclose(src);

    return EXIT_SUCCESS;
}

While others may have more specific suggestions related to the code, here are my observations:

If you're writing this as a Unix filter, make it Unix-y: instead of calling a function rot13, perhaps call it caesar_cipher() and use \$n\$th rotation. This would follow the idea by which Unix systems are characterized: modularity.
With modularity: while in the modern age it is safe to assume most systems have contiguous a-z and A-Z character encoding, a solution which accommodates EBCDIC and other locales than C is worth considering.
Because you're writing a Unix filter, I would look at the source code for GNU implementations of Unix filters (ie head, tail, uniq) to see how they deal with operations on files and error handling (specifically what types of errors could occur with Unix filters).
Ensure you are correctly checking if files exist, permissions, etc. Referencing GNU utils is helpful here as well.
Prefer fputc() instead of fprintf(): you're just printing a character to a stream.

Those are my thoughts. Some of these things you may have already considered, but I hope this was helpful.


Function that calls the database connection and query I made a function that contains the database connection and query. Now I'm always calling this function every time I need to query a database. Is it good or is there another way better?

Database connection:

    /* Database Configuration */
DEFINE( "DATABASE_HOST", "" );
DEFINE( "DATABASE_USERNAME", "" );
DEFINE( "DATABASE_PASSWORD", "" );
DEFINE( "DATABASE_ESSENTIALS", "" );

        /* Database Connection & Data Manipulation */
    function connect_database( $database_id ) {
        try {
            $databases = array( "essentials"  => DATABASE_ESSENTIALS );

            $connection = new PDO( "mysql:host=" . DATABASE_HOST . ";dbname=" . $databases[ $database_id ], DATABASE_USERNAME, DATABASE_PASSWORD );
            $connection->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
        } catch ( PDOException $exception ) {
            trigger_error( $exception->getMessage() );
        }

        return $connection;
    }

    function prepare_and_execute_query( $database_id, $query, $statement_id, $values ) {
        $statement = $this->connect_database( $database_id )->prepare( $query );

        if ( $statement_id == "get_system_configuration" ) {
            $statement->bindParam( ":system_configuration_id", $values[ 0 ] );
        }

        $statement->execute();

        return $statement;
    }

    function get_system_configuration( $system_configuration_id ) {
        return ( ( $system_configuration_row = $this->prepare_and_execute_query( "essentials", "SELECT value FROM `[main]system_configurations` WHERE system_configuration_id = :system_configuration_id", "get_system_configuration", array( $system_configuration_id ) )->fetch() ) ? ( ( !( empty( $system_configuration_row[ "value" ] ) ) ) ? ( $system_configuration_row[ "value" ] ) : ( $system_configuration_row[ "default_value" ] ) ) : ( trigger_error( "Undefined system configuration id" , E_USER_ERROR ) ) );
    }

Sample usage of system configuration function:

<!DOCTYPE html>
<html lang="<?php echo $essentials_global->get_system_configuration( "html_lang" ); ?>">

A couple simple notes:

your code is very hard to read because the lines are so long. Strive to keep each line under ~80 characters.
most of the time in more complex applications, DB connection management is put into a class or classes, not functions. This better allows you to do things like share a single DB connection in different parts of the app, rather than instantiating a new connection every time you need to query the database. That is an expensive operation.
Your method that executes the prepared statement considers only happy path. What if statement preparation or execution fails? Also, it seems strange this this function then "leaks" the underlying PDO implementation to the method caller, requiring the caller to deal with getting the query result set from the PDOStatement object.


Print the length of words as input to a histogram with horizontal bars It works, but I'm sure there's a lot of improvement I could make here. I didn't try to get it to work for words over 10 characters in length, and I also didn't want to use built in properties like .length to do the work for me (I don't know why, I just wanted to figure out how to do it on my own).

#include <stdio.h>

/* print the length of words as input to a histogram with horizontal bars */
int main() {
  int c, i, j;
  int accum = 0;
  int nchar[10];

  for (i = 0; i < 10; ++i)
    nchar[i] = 0;

  while ((c = getchar()) != EOF && accum < 10) {
    if (c != ' ' && c != '\n' && c != '\t') {
      ++accum;
    }
    else {
      ++nchar[accum];
      accum = 0;
    }
  }

  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
  if (nchar[i] > 0) {
    for (j = 0; j < nchar[i]; ++j) {
      putchar('x');
    }
  }
  putchar('\n');
  }
  return 0;
}

Overall this look pretty good. It's very straightforward, and easy to read. Here are a few things I'd work on improving:
Better Naming
I can live with c, i and j. Those are fairly reasonable short names. But I'd rename accum to length or word_len because what you're accumulating is the length of the next word. It's easier to read. Likewise, for nchar, I'd rename it to either histogram or num_chars (or numChars, or whatever your preference is).
Functions Separate Responsibility
Your code is doing 3 things:

Initializing your variables
Getting the words from the user (during which it calculates the histogram)
Displaying the histogram

I'd make at least 2 functions. Initializing the array is fine where it is for now. In the future, you might have more complex data structures you use to keep track of statistics, so you would probably create a initialization function for those.
You could have a function for calculating the histogram and another for displaying it. Something like:
void CalcHistogram(int histogram[10])
{
    int c;
    int length = 0;

    while ((c = getchar()) != EOF && length < 10) {
        if (c != ' ' && c != '\n' && c != '\t') {
            ++length;
        }
        else {
            ++histogram [ length ];
            length = 0;
        }
    }
}

and
void DisplayHistogram(const int histogram[10])
{
      for (int i = 0; i < 10; ++i) {

        printf("%d ", i);

        if (histogram [ i ] > 0) {
            for (int j = 0; j < histogram [ i ]; ++j) {
                putchar('x');
            }
        }
        putchar('\n');
    }
}

As you can see, I also made the loop control variables local to the loop.
By separating the display into its own function, you can replace it with something more sophisticated later (like displaying it graphically, or instead sending it over the network, or whatever), without changing any of the logic of calculating it.
Error Handling
You aren't checking for errors at all. If the user enters a word longer than 10 characters, it suddenly stops taking input and prints out the histogram. That will be surprising to users. I'd at least output a message saying the input was too long, or something like that, if length (accum) gets over 10.
While I'm taking about user interface stuff, I notice that you don't prompt the user for what kind of input to enter. It might be worth adding a simple printf() to tell them what your program expects.


Just a simple guessing game This is a simple guessing game I have made, an improvised one. I will do a GUI version of this shortly.

I'm looking for comments and interesting code ideas.

import random

sec_no = random.randint(0, 10)

ncount = 0

att = 5

while True:
    sec_no = random.randint(0, 10)
    ncount = 0
    att = 4
    minp = input('''
                    1-start
                    2-help
                    3-quit
                 ''')

    if minp.lower() == 'start' or minp == '1':
        while ncount != 4:
            att -= 1

            try:
                uinp = int(input('Guess the no:'))
                if uinp == int(sec_no):
                    print('correct!!!!')
                    break

                else:
                    print('incorrect!!!')
                    print(f'attempts left:{att}')

            except ValueError:
                print('invalid value!!!')
                print(f'attempts left:{att}')

            if ncount == 3:
                print('You lose!!')
                print(f'correct number is :{sec_no}')
                break

    ncount += 1

    elif minp.lower() == 'quit' or minp == '3':
        while True:
            print('are you sure you wanna quit?     [Y/N]')
            linp = input('>')

            if linp.lower() == 'y':
                print('Thanks for playing!!')
                exit()

            elif linp.lower() == 'n':
                break

            else:
                print("I don't understand that!!!")

    elif minp.lower() == 'help' or minp == '2':
        print('''You will be given 3 attempts,within these attempts you'll have to
                 guess the right number from 0-10
                 good luck!!!''')

    else:
        print("I don't understand that!!")

There are few improvements you can do:

Remove the first sec_no = random.randint(0, 10). You are already initializing it inside of the main while loop.
Create a variable that contains minp.lower(). Instead of calculating it over and over - keep it in a variable and avoid code duplication.
Get rid of ncout or att. They are practically saving the same thing, keep only att and change the while and if condition to att > 0.
Add some comments - File comments and function comments.
General python tip: do not write code in main, Use functions. Function for each case, function for main loop, function to check if string is int and the __main__ should look like this:

if __name__ == '__main__':
    main_function()

Except for that - good logic, nice code :)


Unit tests for methods that access the file system Basically I have some methods that access the file system and to avoid that in the unit test I have broken those sections out into a protected virtual method. In the Unit Test I then use Moq to setup those protected methods the way I want. I am just curious if I am going about this correctly or is there a better way? Below is an example method I have done this with however I have several places I do this with.

Method that gets unit tested:

public IEnumerable<ConfigProfile> GetSavedProfiles(string product)
    {
        if(string.IsNullOrEmpty(product))
        {
            throw new ArgumentNullException("product");
        }

        string profileDirectory = _factory.GetCustomProfileDirectory(product);
        List<ConfigProfile> profiles = Enumerable.Empty<ConfigProfile>().ToList();

        _logger.Debug("Getting Saved profiles.");

        IList<FileInfo> files = GetProfilesInDirectory(profileDirectory).ToList();

        if (!files.Any())
        {
            _logger.Debug(string.Format("No custom profiles found for {0}.", product));
            return profiles;
        }

        profiles.AddRange(files.Select(LoadProfile));

        return profiles;

    }

Then I have these two protected methods I have taken out and setup in my unit test:

protected virtual IEnumerable<FileInfo> GetProfilesInDirectory(string directory)
    {
        DirectoryInfo files = new DirectoryInfo(directory);

        return files.EnumerateFiles("*.ecu");
    }

protected virtual ConfigProfile LoadProfile(FileInfo file)
    {
        ConfigProfile profile;
        using (FileStream stream = new FileStream(file.FullName, FileMode.Open))
        {
            profile = _serializer.DeserializeFromDisk(stream);
        }
        return profile;
    }

Another approach is to use a wrapper for the DirectoryInfo, FileInfo and FileStream.  Then your methods can use the wrappers and inject your mocks in the unit tests. Several people have already thought of this and have written the wrappers for you.

The one I like is SystemWrapper. Their sample page shows some good examples.

Of course, you still have the challenge of refactoring your code to inject the wrappers into your method, since creating a new instance of the wrappers within the code leaves you the same problem.  The simpliest approach to injecting wrappers is to pass them in as parameters.  But, since the constructor of the wrappers needs information obtained within the method this is more difficult in your case.  You may consider breaking your method into several methods in a manner such as this...

NOTE:  This code will not compile.  I am just trying to give you an idea of how to break up the method to make it unit testable.  In the end, every method cannot be unit tested.  The key is to break it up into small enough methods so as much code as possible is tested.

public IDirectoryInfoWrap GetDirectoryInfoWrapper(string product)
{
    if(string.IsNullOrEmpty(product))
    {
        throw new ArgumentNullException("product");
    }

    string profileDirectory = _factory.GetCustomProfileDirectory(product);
    return new DirectoryInfoWrap(profileDirectory);
}

public IList<FileInfo> GetProfileFiles(IDirectoryInfoWrap di)
{
    _logger.Debug("Getting Saved profiles.");

    // NOTE:  EnumerateFiles is not in the SystemWrapper so you will have to use an alternative
    IList<FileInfo> files = di.EnumerateFiles("*.ecu").ToList();
    return files;
 }

// Left for you to do...refactor in a manner so that FileSystem is using the IFileSystemWrap
public IEnumerable<ConfigProfile> GetSavedProfiles(IList<FileInfo> files)
{
    List<ConfigProfile> profiles = Enumerable.Empty<ConfigProfile>().ToList();
    if (!files.Any())
    {
        _logger.Debug(string.Format("No custom profiles found for {0}.", product));
        return profiles;
    }

    profiles.AddRange(files.Select(LoadProfile));

    return profiles;

}


Vowels and Consonants From HackerRank's 10 Days of JavaScript, Day2: Loops:

  Complete the vowelsAndConsonants function in the editor below. It has one parameter, a string,s, consisting of lowercase English alphabetic letters (i.e., a through z). The function must do the following:

  First, print each vowel in s on a new line. The English vowels are a, e, i, o, and u, and each vowel must be printed in the same order as it appeared in s.
  Second, print each consonant (i.e., non-vowel) in s on a new line in the same order as it appeared in s.

The following code is in JavaScript. The code runs perfectly fine, just finding a way to make it better.

function vowelsAndConsonants(s) {
    let sp = s.split("");
    let arr1 =[];
    let arr2 =[];
    for(let i=0;i<sp.length;i++)
    {
        if(sp[i]=='a'||sp[i]=='e'||sp[i]=='i'||sp[i]=='o'||sp[i]=='u'){
            arr1.push(sp[i]);
        }else{
            arr2.push(sp[i]);
        }
    }
    for(let i = 0;i<arr1.length;i++){
        console.log(arr1[i]);
    }
    for(let i = 0;i<arr2.length;i++){
        console.log(arr2[i]);
    }
}

From a short review;

arr1 and arr2 are terrible names, we could call them consonants and vowels
accessing sp[i] so often is not efficient, I would store that value in a variable
when dealing with lists, consider going functional (using forEach, map, reduce, join etc.)
sp is not a great name
There is a nicer way to iterate over the characters of a string with for( of )

Finally, I wanted to provide the below as an example where I incorporate the above points;

function vowelsAndConsonants2(s) {
  const vowels = [],
    consonants = [];

  for(const c of s){
    if('aeiou'.includes(c)){
      vowels.push(c);
    }else{
      consonants.push(c);
    }
  }
  console.log(vowels.join("\n"));
  console.log(consonants.join("\n"));
}


Check field values against a preset object I want to check some fields values in a form against an existing object's values. If the values match, change a class value (it will add a sign). The following code is working, but I would like to know if it can be improved.

Preset obj:

var obj = {
    preset_a:{
        input:{
            field_a:"value",
            ...
            field_b:"value",
        },
        select:{
            ...
        },
        radio:{
            ...
        }
    },
    preset_b:{
        input:{
            field_a:"value",
            ...
            field_b:"value",
        },
        select:{
            ...
        },
        radio:{
            ...
        }
    }
};

The script:

$(document).ready(function(){
    for (var preset in obj){
        $.each(obj[preset], function(type){
            var flag = true;
            $.each(obj[preset][type], function(k, v){
                if (type == 'radio'){
                    flag = ((v) == $('#'  + k + ' > input[type="radio"]:checked').val());
                } else {
                    flag = ((v) == $(type + '#' + k).val());
                }
                if (flag == false){
                    return false;
                }
            });
            if (flag == false){
                $(preset + ' i#id').attr('class', 'radio-unchecked');
                return false;
            } else {
                $(preset + ' i#id').attr('class', 'radio-checked on');
            }
        });
    }
});

Inside one of the $.each calls, you could simplify this part:

if (flag == false){
    return false;
}

to this:

return flag;

When the value is true, $.each will continue normally, and stop after it is false, so the behavior remains the same.

Instead of:

if (flag == false) {

It's more common to write:

if (! flag) {

For simplicity, it might be a good idea to flip the condition:

if (flag) {
    $(preset + ' i#id').attr('class', 'radio-checked on');
} else {
    $(preset + ' i#id').attr('class', 'radio-unchecked');
    return false;
}

And to reduce code duplication, I would extract the dom lookup that is common in both branches of the if-else:

var preset = $(preset + ' i#id');
if (flag) {
    preset.attr('class', 'radio-checked on');
} else {
    preset.attr('class', 'radio-unchecked');
    return false;
}

The two innermost loops use $.each,
but the outer loop is a traditional for.
For the sake of consistency, it might be good to use $.each for all.


Basic PHP MVC setup for form submissions I'd like your thoughts on this basic implementation of MVC. The view outputs a form, and when the page is loaded, the controller checks for a form submission and then updates the model accordingly. Finally, the view outputs the form again, grabbing the new values from the model.

One problem with this is that if the user refreshes the page, a submission will still happen because the $_POST['submitted'] flag is still sent. Ideally, I'd like to avoid this - is there a suggestion for using AJAX to make this work?

<?php

class EntryModel {
        public $date;
        public $amount;

        public function update_date($date) {
                $this->date = $date;
        }

        public function update_amount($amount) {
                $this->amount = $amount;
        }
}

class EntryController {
        private $model;

        public function __construct(EntryModel $model) {
                $this->model = $model;
        }

        public function get_submission() {
                if (!isset($_POST['submitted']))
                        return;

                foreach ($_POST as $g => $k)
                {
                        if ($g == 'form_date')
                                $this->model->update_date($k);
                        elseif ($g == 'form_amount')
                                $this->model->update_amount($k);
                }

                echo ("Sucessful submission");
        }

}

class EntryView {
        private $model;
        private $controller;

        public function __construct(EntryController $controller, EntryModel $model) {
                $this->controller = $controller;
                $this->model = $model;
        }

        public function output() {
                $form_output =
                '<form action="index.php" method="POST">

Date: <input type="date" name="form_date" value="' . $this->model->date . '">

Amount: <input type="number" name="form_amount" value="' . $this->model->amount . '">

                        <input type="hidden" name="submitted" value="yes">
                        <input type="submit">
                </form>';

                return $form_output;
        }
}

$model = new EntryModel();
$controller = new EntryController($model);
$view = new EntryView($controller, $model);

$controller->get_submission();

echo $view->output();

?>

Templates

Your HTML code should be separated out into a template file. You should do your best to avoid mixing HTML into your PHP, as your HTML will inevitably grow, and your PHP will get messier as a result.

Post/Get/Redirect

The pattern you are looking for is Post/Get/Redirect. You should always follow this pattern. When a user submits a form using POST, you should process the form, and then if it was successful, redirect them back to some page (possibly the form page again) with an added URL parameter.

E.g. http://example.com/contact-us?submitted=true

Then in your form code, you can check if the URL parameter is present, and show a success message.


Program to print all evironment variables I'm learning x86 assembly on Linux and this is the first program I wrote, so I'm looking for all kinds of ways to improve it.

; Build using these commands:
;   nasm -f elf printenv.asm
;   ld -o printenv printenv.o -m elf_i386 -s

section .data
section .bss

section .text

global _start

_start:
  pop ecx                 ; load argument count
  lea esp,[esp+ecx*4+4]   ; move past every argument and null ptr

  PopEnv:
  pop esi                 ; get next env variable
  cmp esi,0               ; null means end
  je Exit                 ; exit if null

  ; calculate length
  mov ecx,0ffffffffh      ; max length
  xor eax,eax             ; search for null byte
  mov edi,esi             ; where to look for
  repne scasb             ; search
  dec edi                 ; edi points one past the null
  mov byte [edi],0ah      ; replace null with EOL
  sub edi,esi             ; length
  mov edx,edi             ; set arg for sys_write
  inc edx                 ; include EOL

  ; print env to console
  mov eax,4               ; sys_write
  mov ebx,1               ; stdout
  mov ecx,esi             ; location
  int 80h                 ; call
  cmp eax,edx             ; check return value
  jb ExitFailure          ; error

  jmp PopEnv              ; get another env

  Exit:
  mov eax,1               ; sys_exit
  mov ebx,0               ; EXIT_SUCCESS
  int 80h                 ; make the call

  ExitFailure:
  mov eax,1               ; sys_exit
  mov ebx,-1              ; EXIT_FAILURE
  int 80h                 ; make the call

There's always a question of what 'improve' might mean.  I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.'  That said, if I were doing a code review, these are some of the things I'd mention:

You have lots of very clear comments.  That's a good thing, especially in assembler.  I can't count the number of times I've read code and thought "What the heck was this guy THINKING?"  This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
You don't have a comment that describes what the program does.  While your SE post has a (misspelled) description, the code does not.
While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.'  Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
While it works, I wouldn't get into the habit of using repne.  It's not the best performing way to do this kind of loop (see https://stackoverflow.com/a/34141835/2189500).  BTW, this is one of the reasons to use a high level language (like C).  Compilers already know all this stuff and (try to) use the best possible construct.
I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
I might consider changing the jb ExitFailure.  jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away.  But since your code is small, you could do:

a

jnb PopEnv              ; get another env

ExitFailure:
mov eax,1               ; sys_exit
mov ebx,-1              ; EXIT_FAILURE
int 80h                 ; make the call

Exit:
mov eax,1               ; sys_exit
mov ebx,0               ; EXIT_SUCCESS
int 80h                 ; make the call

In summary, my comments are mostly just piddly 'style' stuff.


Either monad in C++11 I'm using C++11 with https://github.com/SmingHub/Sming. It has no support of std lib or exceptions. I wanted to use Either monad to return an error or result from my functions.
I implemented it using class hierarchy. I'm not a native C++ dev so I would be glad for the code review.
#pragma once

template <class L, class R>
class Either
{
    virtual bool isLeft();
    virtual bool isRight();

    virtual bool matchLeft(L& x);
    virtual bool matchRight(R& x);
};

template <class L, class R>
class Left : public Either<L, R> {
public:
    L left;

    Left(L left): left(left) {}
    constexpr bool isLeft() { return true; }
    constexpr bool isRight() { return false; }

    bool matchLeft(L& x)
    {
        x = left;
        return true;
    }
    bool matchRight(R& x) { return false; }
};

template <class L, class R>
class Right : public Either<L, R> {
public:
    R right;

    Right(R right): right(right) {}
    constexpr bool isLeft() { return false; }
    constexpr bool isRight() { return true; }

    bool matchLeft(L& x) { return false; }

    bool matchRight(R& x) {
        x = right;
        return true;
    }
};

Your Either-monad suffers from a fatal flaw:
It is impossible to return an Either which might be either possibility.
You can only return a Left or a Right, an Either must per force be a pointer or reference to one of those.
Without help from the compiler in automatically deducing a return-type of Either<T, U> if it finds returns of a combination of Either<T, U>, Left<T> and Right<U> sufficient to deduce T and U, or at least adding pattern-matching, Left and Right are pretty much useless.
In the end, that just leaves Either, which is now only a poor-mans substitute for full-fledged std::variant of two.


Linq-to-Sql Contains an int? inside a list of int Curious if there is a better way to write a repository method that receives argument List<int> and checks if the database Contains an int?.

public List<int> GetNumbersByNumbers(IEnumerable<int> numbers)
{
    var convertedNumbers = numbers.Select(m => (int?)m);
    return Context.TblNumbers
        .Where(m => convertedNumbers.Contains(m.Number))
        .Select(m => m.Number.GetValueOrDefault())
        .ToList();
}

Another way to do this is to remove NULL values from the comparison to begin with, like so:

return Context.TblNumbers
              .Where(m => m.Number != null)
              .Where(m => convertedNumbers.Contains(m.Number.Value))
              .Select(m => m.Number.Value)
              .ToList();

I think this also reads more naturally since you only have the chance to match on non-null values anyway, instead of GetValueOrDefault() which implies you might need to get a default value.


Guess the presents someone will get by comparing wishlist attributes #
# Guess the presents someone will get
# We compare a `wishlist` with many criteria to the `presents` and return an array with the names of the matching products.
#
def guess_gifts(wishlist, presents)

  wishlist.map do |w|
    w[:name] if presents.include? w.reject { |key| key == :name }
  end.compact.uniq

end

guess_gifts([

{:name => "mini puzzle", :size => "small", :clatters => "yes", :weight => "light"},
{:name => "toy car", :size => "medium", :clatters => "a bit", :weight => "medium"},
{:name => "card game", :size => "small", :clatters => "no", :weight => "light"}

],
[

{:size => "medium", :clatters => "a bit", :weight => "medium"},
{:size => "small", :clatters => "yes", :weight => "light"}

])

In particular, I'd like to know whether w.reject { |key| key == :name } is the best way to remove the :name item.  Is there any shorter or faster way to do so?  I am often very surprised with Ruby capacity to shorten processes, so I'm wondering.

It's nice that you were able to implement guess_gifts as a single expression.

I don't recommend writing it that way, though.  For every item in the wishlist, you will scan the entire presents list for a match.  Wouldn't it be nice to just do a lookup instead?  You could do such a lookup if the wishlist were in a more convenient format.

def guess_gifts(wishlist, presents)
  # Transform the wishlist, mapping attributes to a list of names
  wishmap = Hash.new { |hash, attributes| hash[attributes] = [] }
  wishlist.each { |w| wishmap[w.reject { |k| k == :name }].push(w[:name]) }

  # Then just do the lookups
  presents.flat_map { |attributes| wishmap[attributes] }
end

Another benefit is that we're doing a map on presents rather than on wishlist, so I think we might not need to call uniq on the results.


Identifying equivalent lists Title:
Identifying equivalent lists

I am using lists as keys, and therefore want to identify equivalent ones.

For example,

[0, 2, 2, 1, 1, 2]
[4, 0, 0, 1, 1, 0]

are 'equivalent' in my book. I 'equivalify' the list by checking the list value with the highest count, and set that to 0, then the next highest count to 1, and so on and so forth. It's done with this code below, which does the job properly but is fairly slow - and I am wondering if there is a way of doing the same job yet I haven't seen it yet.

for key in preliminary_key_list:
    key = list(key) # convert from immutable tuple to mutable list

    i = 0
    translation = {}
    counts = [[j, key.count(j)] for j in set(key)]
    counts_right = [j[1] for j in counts]
    for count in set(key):
        translation[counts[counts_right.index(max(counts_right))][0]] = i
        counts_right[counts_right.index(max(counts_right))] = 0
        i += 1

    for i in range(len(key)):
        key[i] = translation[key[i]]

for key in preliminary_key_list:
    key = list(key) # convert from immutable tuple to mutable list

You don't need to do this. Rather then creating a modifiable list, just use the tuple and construct a new tuple at the end

    i = 0
    translation = {}
    counts = [[j, key.count(j)] for j in set(key)]

This should be a list of tuples, not a list of lists.

    counts_right = [j[1] for j in counts]

    for count in set(key):

Use for i, count in enumerate(set(key)): to avoid having to manage i yourself.

You use set(key) twice, which will force python to do work twice. Also, there is no guarantee you'll get the same order both times.

        translation[counts[counts_right.index(max(counts_right))][0]] = i
        counts_right[counts_right.index(max(counts_right))] = 0

These lines are really expensive. max will scan the whole list checking for the highest value. .index will then rescan the list to find the index for the value.
Then you do the whole thing over again on the next line!

You'd do far better as @GlenRogers suggests to sort by the counts.

        i += 1

    for i in range(len(key)):
        key[i] = translation[key[i]]

Here using a comprehension would be better (I think):

 key = tuple(translation[item] for item in key)

Here is how I'd write the code:

ret = []
for key in preliminary_key_list:
    counts = sorted(set(key), key = lambda k: -key.count(k) )
    translation = { val: idx for idx, val in enumerate(counts) }
    ret.append([translation[k] for k in key])
return ret


Check if all leaves are at same level in a binary tree I have written this code for checking whether all leaves are in the same level or not, in a binary tree. Any suggestion or improvement is appreciated.

import java.util.LinkedList;
import java.util.Queue;

public class Methods {

    //* ---- Method for check whether all leaves are in the same level or not --- *
    public static boolean leavesIn1Level(Node root){

        if(root == null){
            return false;
        }
        if(root.left == null && root.right == null){
            return true;
        }
        Queue<Node> q = new LinkedList<Node>();
        q.add(root);

        int levelNumber = 0;
        int height = height(root);

        while(true){

            int nodeCount = q.size();
            if(nodeCount == 0){
                break;
            }
            while(nodeCount > 0){

                Node current = q.remove();
                nodeCount--;

                if(current.left == null && current.right == null){
                    if(height != levelNumber){
                        return false;
                    }else{
                        return true;
                    }
                }
                if(current.left != null){
                    q.add(current.left);
                }
                if(current.right != null){
                    q.add(current.right);
                }
            }
            levelNumber++;
        }
        return false;
    }

    //* ---- Method for finding height of a binary tree ---- *
    public static int height(Node root){

        if(root == null){
            return -1;

        }else{

            return Math.max(height(root.left), height(root.right))+1;
        }
    }

    public static void main(String[] args) {

        Node root1 = new Node(8);                      //              8
        root1.left = new Node(9);                      //             / \
        root1.right = new Node(5);                     //            9   5
        root1.left.left = new Node(4);                 //           /   /
        root1.right.left = new Node(1);                //          4   1
        root1.right.left.left = new Node(3);           //             /
        root1.right.left.left.left = new Node(2);      //            3
                                                       //           /
                                                       //          2

        Node root2 = new Node(1);                      //              1
        root2.left = new Node(2);                      //             /  \
        root2.right = new Node(3);                     //           2     3
        root2.left.left = new Node(4);                 //          / \   /
        root2.left.right = new Node(5);                //        4    5 6
        root2.right.left = new Node(6);

        System.out.println(leavesIn1Level(root1));
        System.out.println(leavesIn1Level(root2));
   }
}

Node class:

public class Node {
    int data;
    Node left;
    Node right;
    public Node(int d){
        data = d;
        left = null;
        right = null;
    }
}

This code is different from:
Check if all leaves are at same level

Okay lets start with the name of your function: leavesIn1Level(...). It does explain what this method does, but apparently it is not very clear - even to yourself - therefore you felt the need to add a comment. How a bout the name areAllLeavesInSameLevel(..). This explais what it does and makes your comment useless.

Next point (a compliment): You used a Queue, which is the data-structure that fits your needs perfectly. Good job.

One thing I noticed: The line node.left == null && node.right == null is used twice in your short piece of code and therefore - code duplication. How do we get rid of it? Well... What does that line of code do? It checks whether a Node is a Leaf. So why don't we just name it that way?

private static boolean isLeaf(Node node)
{
    return node.left == null && node.right == null;
}

Boom. Code duplication gone. And the code explains exactly what it does.

Now what I realized, is that you have a very deep level of abstraction in this function, where you mix what is done with how its done. Lets try to get rid of this:

if(current.left != null){
    q.add(current.left);
}
if(current.right != null){
    q.add(current.right);
}

This piece of code explains how something is done. But what is done? Children of a node are added to a queue. Why don't we just name it that?

private static void addChildrenToQueue(Node node, Queue<Node> queue)
{
    if(node.left != null){
        queue.add(node.left);
    }
    if(node.right != null){
        queue.add(node.right);
    }
}

Lets take a look at what we have now:

public static boolean areAllLeavesInSameLevel(Node root){

    if(root == null){
        return false;
    }
    if(isLeaf(root)){
        return true;
    }
    Queue<Node> q = new LinkedList<Node>();
    q.add(root);

    int levelNumber = 0;
    int height = height(root);

    while(true){

        int nodeCount = q.size();
        if(nodeCount == 0){
            break;
        }
        while(nodeCount > 0){

            Node current = q.remove();
            nodeCount--;

            if(isLeaf(current)){
                if(height != levelNumber){
                    return false;
                }else{
                    return true;
                }
            }
            addChildrenToQueue(current,q);
        }
        levelNumber++;
    }
    return false;
}

Now I'm not particularly happy with the name q. What is inside that queue? It's nodes that need to be checked - in other words: remainingNodes. Lets just call it that.

Take a look at this:

while(true){
    int nodeCount = remainingNodes.size();
    if(nodeCount == 0){
        break;
    }
    ...
}
return false;

What does it do? If the queue is empty it jumps out of the loop and then returns false. How about we move it around a bit to get rid of that while(true):

while(!remainingNodes.isEmpty()){
    int nodeCount = remainingNodes.size();
    ...
}
return false;

Aaaand the while(true) is gone and replaced with a much better phrased condition. While the Collection of remaining nodes is not empty do something. At this point we can argue that the queue could have the even better name queueOfRemainingNodes. Some would say it's too long. I like it, but that's up to you to decide.

Let's take a look at the second while:

int nodeCount = queueOfRemainingNodes.size();
while(nodeCount > 0){
    nodeCount--;
    ...
}

To me this looks like the classic case for a for-loop instead:

for(int nodeCount = queueOfRemainingNodes.size(); nodeCount > 0; nodeCount--)
{
    ...
}

Okay. Now what does this inner for-loop actually do? It checks whether any node, that is currently in the queue is a leaf and if it is not, it adds its children to the queue. If however one is a leaf, it does the following check:

if(height != levelNumber){

What? Is height equal to levelNumber? What is levelNumber? And which height? By looking up we find out that height is the height of the tree and levelNumber is the number of the level we are currently looking at. Let's give these two some more expressive names:

int currentLevelNumber = 0;
int treeHeight = height(root);

So where are we now? Let's take a look at the full code:

public static boolean areAllLeavesInSameLevel(Node root){

    if(root == null){
        return false;
    }
    if(isLeaf(root)){
        return true;
    }
    Queue<Node> queueOfRemainingNodes = new LinkedList<Node>();
    queueOfRemainingNodes.add(root);

    int currentLevelNumber = 0;
    int treeHeight = height(root);

    while(!queueOfRemainingNodes.isEmpty()){

        for(int nodesOnCurrentLevel = queueOfRemainingNodes.size(); nodesOnCurrentLevel > 0; nodesOnCurrentLevel--){

            Node current = q.remove();

            if(isLeaf(current)){
                if(treeHeight != currentLevelNumber){
                    return false;
                }else{
                    return true;
                }
            }
            addChildrenToQueue(current,queueOfRemainingNodes);
        }
        currentLevelNumber++;
    }
    return false;
}

Wow. Just realized how long this post got. Sorry, I could go on for hours and remove this second (and third) level of intendation and find more expressive names for everything, but I think this gives you an impression on what you can do.


Convert boolean list to list of base10 integers Context:

I'm coding up a simple genetic optimization algorithm from first principles, and have decided to use boolean/binary strings as genes so that I can flip bits on or off as a form of mutation. In order to evaluate the fitness of the solutions that it generates I need to be able to decode the binary strings produced into the integer values that I'm optimizing for. The genetic optimization technique that I'm using returns it's individuals as a list, so I am extracting the integers as follows:

Problem:

Take the boolean/binary list...

l = [1, 0, 1, 0, 0, 1, 1, 1, 0, 0, 1, 0, 1, 0, 1, 0, 0, 0, 1, 1, 1, 0, 1, 1]

...split it into 8-bit sublists...

[[1, 0, 1, 0, 0, 1, 1, 1], [0, 0, 1, 0, 1, 0, 1, 0], [0, 0, 1, 1, 1, 0, 1, 1]]

...concatenate each sublist into a string...

[10100111, 101010, 111011]

...convert those strings from binary to decimal...

[167, 42, 59]

...then scale those integers between 50 and 150...

[115, 66, 73]

My solution:

[int(int(str(bin), 2)/2.55)+50 for bin in [int(''.join(\
    map(str,num))) for num in [l[i:i +8] for i in range(0, len(l), 8)]]]

This feels like a very ugly way to go tackle the problem - can anyone come up with a nicer way to go about it?

The individual steps that you describe in the Problem section are each simple and easy to follow. Yet you write your code in basically a single line. To understand it I had to tear it apart like you did in your "explanation for humans":

scaled = [
    int(int(str(bin), 2)/2.55)+50
        for bin in [
            int(''.join(map(str, num)))
                for num in [
                    l[i : i+8] for i in
                        range(0, len(l), 8)]]]

I have also reformatted the code to better see the underlying structure. Together with your explanation, this works for understanding it.

But since you chose to not include the explanation within your code, I would have no chance to understand it by reading it alone.

Therefore, I find it easier to understand when you name the intermediate steps:

chunks = [l[i : i+8] for i in range(0, len(l), 8)]
binaries = [int(''.join(map(str, chunk))) for chunk in chunks]
numbers = [int(str(binary), 2) for binary in binaries]
scaled = [round(50 + (150 - 50) * number / (255 - 0)) for number in numbers]

There is a pythonic shortcut for step 1, and steps 2 and 3 can be combined into one:

chunks = zip(*[iter(l)]*8)
numbers = [sum(bit << shift for bit, shift in zip(chunk, reversed(range(8))) for chunk in chunks]
scaled = [round(50 + (150 - 50) * number / (255 - 0)) for number in numbers]

The aim of PEP8 is to ensure readable code. Having too many different ideas in a single line of code does not count as readable to me. Therefore I prefer the form with the intermediate steps.

Sure, the broken down version of the code is longer, but the reader of your code can take a deep breath after each step and inspect the intermediate result, just as you did in your explanation. Therefore the code should look this way.

By the way, I replaced the very last int with round since I think it is more appropriate. Decide for yourself. I also replaced the magic number 2.55 with the numbers from your explanation. This makes the numbers less magic.


Pig Latin translator in C I am learning C, and wrote this pig-latin translator.

If no argument is given, it operates on standard input. Otherwise, it operates on the arguments given.

Here are some examples:

$ echo "The quick brown fox jumps over Vladmir's lazy young Yggdrasil!" | ./piglatin
Ethay ickquay ownbray oxfay umpsjay overway Admir'svlay azylay oungyay Yggdrasilway!

$ ./piglatin "Now is the time, for all good men to come to the aid of their party"
Ownay isway ethay imetay, orfay allway oodgay enmay otay omecay otay ethay aidway ofway eirthay artypay

#include <err.h>
#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define SUFFIXC  "ay"
#define SUFFIXV  "way"

static void usage(void);
static void piglatin(const char *s);
static int isvowel(const char *s);

/* Encodes English-language phrases into Pig Latin. */
int
main(int argc, char *argv[])
{
    int c;

    while ((c = getopt(argc, argv, "h")) != -1) {
        switch (c) {
        case 'h':
        default:
            usage();
            break;
        }
    }
    argc -= optind;
    argv += optind;

    if (argc == 0) {
        char *line = NULL;
        size_t linesize = 0;
        ssize_t linelen;

        while ((linelen = getline(&line, &linesize, stdin)) != -1) {
            if (line[linelen - 1] == '\n')
                line[linelen - 1] = '\0';
            piglatin(line);
        }
        free(line);
        if (ferror(stdin))
            err(EXIT_FAILURE, "stdin");
    } else {
        while (argc-- > 0)
            piglatin(*argv++);
    }
    if (ferror(stdout))
        err(EXIT_FAILURE, "stdout");

    return EXIT_SUCCESS;
}

/* translate s into Pig Latin */
static void
piglatin(const char *s)
{
    const char *p, *onset, *root, *end, *suffix;
    bool upper;

    while (*s != '\0') {
        while (!isalpha(*s) && *s != '\'' && *s != '\0')
            putchar(*s++);

        upper = false;
        if (isupper(*s))
            upper = true;

        if (*s == '\0')
            break;

        onset = s;

        while (isalpha(*s) && !isvowel(s)) {
            if (*s == 'q' && *(s+1) == 'u')
                s++;
            s++;
        }
        root = s;

        while (isalpha(*s) || *s == '\'')
            s++;
        end = s;

        suffix = (onset == root) ? SUFFIXV : SUFFIXC;

        for (p = root; p != end; p++) {
            if (p == root && upper)
                putchar(toupper(*p));
            else
                putchar(*p);
        }

        for (p = onset; p != root; p++) {
            if (p == onset && upper)
                putchar(tolower(*p));
            else
                putchar(*p);
        }

        printf("%s", suffix);

    }

     printf("\n");
}

/* test if first letter of s is a vowel */
static int
isvowel(const char *s)
{
    switch (tolower(*s)) {
    case 'a': case 'e': case 'i': case 'o': case 'u':
        return 1;
    case 'y':
        return (isvowel(s+1) ? 0 : 1);
    }
    return 0;
}

static void
usage(void)
{
    (void) fprintf(stderr, "usage: piglatin [phrase...]\n");
    exit(EXIT_FAILURE);
}

Inadvertent case dependency?

Should not the below code also work with 'Q', 'U'?

if (*s == 'q' && *(s+1) == 'u')

char functions (advanced issue)`

The Std C library functions treat  char * data as if it was accessed by unsigned char *.

This is important in select cases.

tolower(int ch), is...() are well defined for values in the unsigned char range and EOF, else UB.  When *s < 0, tolower(*s) is a problem.  Casting to (unsigned char) fixes that.

isvowel(const char *s) {
  // switch (tolower(*s)) {
  switch (tolower((unsigned char) *s)) {

With some extended ASCII encoding this is useful, yet with UTF8, the issue moot as tolower() is wholly inadequate.

Deeper pedantic detail

(unsigned char) *s is the wrong solution with non-2's complement and signed char (not seen these days) as code should access the data an unsigned char.

isvowel(const char *s) {
  const char *us = (const char *) s;
  switch (tolower(*us)) {

With 2's complement the result is the same with either approach.  IAC, I doubt much C code these days would survive a new non-2's complement integer encoding so no real reason to worry about this deep detail.

Formatting

Code lacks {} in simple blocks.  I find the later style easier to review and maintain.  As with such style issues, code to your group's coding standard.

} else {
    while (argc-- > 0)
        piglatin(*argv++);
}

vs.

} else {
    while (argc-- > 0) {
        piglatin(*argv++);
    }
}

Consider local variables, initialization and code reduction

//bool upper;
//...
//    upper = false;
//    if (isupper(*s))
//        upper = true;
      bool upper = isupper(*s);


Modular, conflict free, BEM-style CSS I want to standardise the CSS I use, and am leaning towards bem — I know I'll get a lot of benefit from the predictability of its pattern.

I have hacked their formula ever so slightly - I don't include the B part of the classname on child elements. For me this makes the html easier to write, and maintain.

Pure BEM

.Slideshow {}
.Slideshow__slide {}
.Slideshow__image {}

My Modified BEM

.Slideshow {}
.Slideshow .slide {}
.Slideshow .image {}

The only advantage I see to using 'pure' BEM over my modified version is the avoidance of conflict between child elements of different blocks (e.g. .Slideshow .image and .AdvertBanner .image).

This is not a problem for me, because I use LESS, nesting block level rules automatically.

Can anybody suggest other advantages to using 'pure' BEM over my modified version?

What happens if one of your slides is itself a block that contains an image, such as an article. Consider this markup:

They seem fine on their own and for six months they might be, but it's perfectly reasonable for the
 block to be nested in the
 slide show block, at which point any styles defined on .slide-show .image will effect the article's images too.

By building up modules using BEM and sticking to the naming conventions you guarantee unique classes that won't clash regardless of the order or hierarchy. You know the styles won't leak or be leaked on and you know that you can move the block anywhere in the hierarchy without leakage.


Create an array with at least a given number of bits I use this declaration for an array which must contain a bit for every minute in a day:

UCHAR minutesOfDay[(60 * 24 / CHAR_BIT) + (((60 * 24) % CHAR_BIT) ? 1 : 0)];

However, this look pretty awful. Is there a simpler way to state the same? It can be in C or C++.

Yes, wrap it in a macro:

#ifdef CHAR_BIT
    #define BIT_PER_MIN_IN_DAY ((60 * 24 / CHAR_BIT) + (((60 * 24) % CHAR_BIT) ? 1 : 0))
#else
    #error "Platform not supported"
#endif

UCHAR minutesOfDay[BIT_PER_MIN_IN_DAY];


Asynchronously fetching data from the database The following C# code is written to fetch the Categories data from SQL Server Database using Asynchronous Task for HttpGet. The Dapper library is used as ORM.

I need help to identify whether the Async Task implementation is correct or any better way to implement.

CategoriesController.cs

public class CategoriesController : Controller
{
  [HttpGet]
  public async Task<IActionResult> FindList(CategorySearchModel searchModel)
  {
    var results = await new CategoryQueryService().FindList();

    return GetHandler(results);
  }

  IActionResult GetHandler(object results)
  {
      return new OkObjectResult(new { results });
  }
}

CategoryQueryService.cs

public class CategoryQueryService : BaseQueryDataStoreAsync<CategoryQueryModel>
{
  public override Task<IEnumerable<CategoryQueryModel>> FindList()
  {
    const string dbConnectionString = "-- db connection string here --";
    const string sql = "SELECT CategoryId, CategoryName FROM Category ORDER BY CategoryName ASC";

    return QueryAsync(dbConnectionString, sql);
  }
}

CategoryQueryModel.cs

public class CategoryQueryModel {
  public int CategoryId { get; set; }
  public int CategoryName { get; set; }
}

BaseQueryDataStoreAsync.cs

using Dapper;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Threading.Tasks;

public abstract class BaseQueryDataStoreAsync<T> where T : class
{
  public abstract Task<IEnumerable<T>> FindList();

  public async Task<IEnumerable<T>> QueryAsync(string dbConnectionString, string sql)
  {
    using (var connection = new SqlConnection(dbConnectionString))
    {
      return await connection.QueryAsync<T>(sql, conditions: null, transaction: null, commandTimeout: null, commandType: null);
    }
  }
}

Your async/await usage doesn't look fine because the chain it's incomplete. FindList should be async and named FindListAsync and it should await the result from QueryAsync.

There are also a few other things that botter me:

since BaseQueryDataStoreAsync is an abstract class then QueryAsync should probably be protected and not public - otherwise someone can use it for any query
the CategoryQueryService should be injected
the GetHandler doesn't look very useful
making the dbConnectionString a const is a terrible idea - you could/should use appSettings.json to store it and IConfiguration or IOptions to get it
setting QueryAsync's default parameters to null where they are null anyway (source) is pretty pointless - instead just use the overload with a single parameter.


PHP - Packing widgets into the fewest number of boxes, plus minimum order quantity The problem is this:

A company supplies widgets in a set of pack sizes:

250
500
1000
2000
5000

Customers can order any number of widgets, but the following rules apply:

Only whole packs can be sent and …
No more widgets than necessary should be sent and …
The fewest packs possible should be sent

Some examples showing the number of widgets ordered and the required pack quantities and sizes to correctly fulfill the order:

1 (1 x 250)
251 (1 x 500)
501 (1 x 500 and 1 x 250)
12001 (2 x 5000 and 1 x 2000 and 1 x 250)

I’ve looked at some algorithms (greedy coin change, LAFF, etc.) as these seem to provide similar solutions, but can’t seem to see how to adapt them to the above.

For example:

<?php

function countWidgets($amount)
{
    $packs = array(5000, 2000, 1000, 500,
                  250);
    $packCounter = array(0, 0, 0, 0, 0);
    $packsCount = count($packs);

    // loop packs
    for ($i = 0; $i < $packsCount; $i++)
    {
        if ($amount >= $packs[$i])
        {
            $packCounter[$i] = intval($amount /
                                      $packs[$i]);
            $amount = $amount -
                      $packCounter[$i] *
                      $packs[$i];
        }
    }

    // if remainder
    if ($amount > 0) {
        // and if smallest pack size populated
        if ($packCounter[4] == 1) {
            // clear smallest pack size
            $packCounter[4] = 0;
            // increment next biggest pack size
            $packCounter[3] += 1;
        } else {
            // increment smallest pack size
            $packCounter[4] +=1;
        }
    }

    // Print packs
    echo ("Pack ->"."\n");
    for ($i = 0; $i < $packsCount; $i++)
    {
        if ($packCounter[$i] != 0)
        {
            echo ($packs[$i] . " : " .
                  $packCounter[$i] . "\n");
        }
    }
}

$amount = 251;
countWidgets($amount);

OK, I agree that this is a bit harder than I assumed yesterday. You basically have two opposing demands:

No more widgets than necessary should be sent.
The fewest packs possible should be sent.

You cannot fulfill both. So if I tried to send 1200 widgets, rule 2 says I should send a pack of 2000, however, rule 1 says I should send 2 packs: one of 1000 and one of 250. Which rule should prevail?

I chose that rule 1 should prevail in the solution below. The reason is that no customer wants more widgets than absolutely necessary.

$packSizes = [ 250,
               500,
              1000,
              2000,
              5000];

function optimizePacks($packSizes,$number)
{
    rsort($packSizes);
    $requiredPacks = array_fill_keys($packSizes,0);
    foreach ($packSizes as $size) {
        $packs = floor($number/$size);
        if ($packs > 0) {
            $requiredPacks[$size] = $packs;
            $number -= $packs*$size;
        }
    }
    if ($number > 0) $requiredPacks[min($packSizes)]++;
    return $requiredPacks;
}

$packs = optimizePacks($packSizes,6666);

print_r($packs);

This will work for any array of pack sizes and any number of widgets. The output of this code is:

  Array (
      [5000] => 1
      [2000] => 0
      [1000] => 1
      [500] => 1
      [250] => 1 )

Which is one pack more than rule 2 would demand (one of 5000 and two of 1000). It would, of course, be possible to let rule 2 prevail, but I cannot fulfill both rules.


Calling restful web services efficiently I need to use this code to fetch data from web service and show them in a site properly but after I request for the webservice I am unable to load them fast. Site is loading very slow.

<?php

$jRequestURLPrefix = 'http://demo.4ds.com/rema/1.1/';

$menu_json_url  = $jRequestURLPrefix."rules/origins/b2c.json";

$menu_data      = get_json_data($menu_json_url);

/* Function get_json_data definition */

function get_json_data($json_url, $return_array = true, $print_array = false, $curl = true )
{
    $jsonString = '';
    $data       = array();
    if (!$curl)
    {
        /*
         *  if !$curl, use "file_get_contents" method
         *  to get JSON encoded string
        */
        $jsonString = file_get_contents($json_url);

    }
    else
    {
        /*
         *  if $curl, use "curl" method
         *  to get JSON encoded string
        */

        // Initializing curl
        $ch = curl_init( $json_url );

        // Configuring curl options
        $options = array(
            CURLOPT_RETURNTRANSFER => true,
            CURLOPT_HTTPHEADER => array('Content-type: application/json') ,
        );

        // Setting curl options
        curl_setopt_array( $ch, $options );

        // Getting JSON encoded string
        $jsonString =  curl_exec($ch);
    }

    // convert the JSON encoded string into a PHP variable(array)
    //if($return_array)
        $data = json_decode($jsonString, $return_array);

    // $print_array == true, print the array
    if($print_array)
    {
        echo '<pre>';
        print_r($data);
        echo '</pre>';
    }

    return $data;

}

?>

After using the above procedure I am getting the response very slow.

I think its the API, I've just benchmarked it using

$json = file_get_contents('http://demo.4ds.com/rema/1.1/rules/origins/b2c.json');
$data = json_decode($json);

I only ran 10 attempts but they seem to take between 1 and 5 seconds each.

php test.php  0.03s user 0.01s system 2% cpu 1.574 total
php test.php  0.03s user 0.01s system 1% cpu 3.303 total
php test.php  0.04s user 0.01s system 0% cpu 5.881 total
php test.php  0.04s user 0.01s system 2% cpu 1.646 total
php test.php  0.03s user 0.01s system 2% cpu 1.478 total
php test.php  0.03s user 0.01s system 2% cpu 1.516 total
php test.php  0.03s user 0.01s system 2% cpu 1.590 total
php test.php  0.04s user 0.01s system 1% cpu 3.659 total
php test.php  0.03s user 0.01s system 1% cpu 3.582 total
php test.php  0.03s user 0.01s system 2% cpu 1.526 total


Getting a hash string for a very large file After reading about large files and memory problems, I'm suspecting that my code below may be inefficient because it reads the entire files into memory before applying the hash algorithm. Is there a better way?

chunk_size = 1024
hasher = hashlib.md5()
while True:
    try:
        data = f.read(chunk_size)
    except IOError, e:
        log.error('error hashing %s on Agent %s' % (path, agent.name))
        return {'error': '%s' % e}
    if not data:
        break
    hasher.update(data)
hash_string = hasher.hexdigest()

Well, MD5 is considered to be mostly cracked, and should be avoided.  But anyway…

The comma in except IOError, e is considered deprecated.  except IOError as e is preferred since Python 2.6, and required in Python 3.x.

I don't think that your code is inefficient.  It certainly doesn't read the entire file into memory at once.  A larger chunk size wouldn't hurt.  I think that 8192 bytes would be reasonable: it's approximately a memory page on some machines and approximately the size of a jumbo frame.

I think it would be stylistically beneficial to separate the file-reading code from the hash-calculating code using a generator.  Alternatively, using mmap wouldn't be a bad idea.

def read_chunks(file_handle, chunk_size=8192):
    while True:
        data = file_handle.read(chunk_size)
        if not data:
            break
        yield data

def md5(file_handle):
    hasher = hashlib.md5()
    for chunk in read_chunks(file_handle):
        hasher.update(chunk)
    return hasher.hexdigest()

try:
    with open(sys.argv[1], 'rb') as f:
        hash_string = md5(f)
except IOError as e:
    log.error(…)


Rails model for aggregate data instead of a real table I have a rails app with a Project model. Each project has usages which is numeric data.
I need to get all the usage for each month and year. I also need to get usage for specific months and years across multiple projects.
Instead of writing one large service file with raw SQL queries to handle this, I have decided to extract a lot of the code to a model which isn't based on any table, but instead gets the data by using certain SQL aggregation features.
This allows me to take (almost) full advantage of ActiveRecord in consumers to extend the default query to get specific fields (e.g. where the month is May or the project is project #15).
Here is the example of the code I wrote:
class MonthlyUsage < ActiveRecord::Base
  self.table_name = 'projects'

  default_scope do
    select(select_monthly_data_and_dates)
      .group('year', 'month', '"projects"."id"')
  end

  def self.select_monthly_data_and_dates
    <<~SQL.squish
      "projects"."id" AS "project_id",
      SUM("usages"."usage") AS "monthly_usage",
      date_part('year', "usages"."timestamp")::INT AS "year",
      date_part('month', "usages"."timestamp")::INT AS "month"
    SQL
  end
end

In reality there are a few joins and extra steps to this code which I'm omitting here since they aren't relevant.
Basically I'm of two minds on this. On the one hand I think it allows for powerful querying and fits the definition of a model, but on the other hand I think it might be too "hacky" and not really much clearer than just using a service file (e.g. setting the table name as 'projects' even though that's not really the real table name). That's why I'm asking for code review here, to get some opinions on whether this is good or bad, as well as how you would do it.

I agree with you that this does not really fit a service nor a model. This might be more like a Query object.
class MonthlyUsageQuery
  def intialize(relation: Project.all)
    @relation = relation
  end

  def all
    relation.select(select_monthly_data_and_dates)
      .group('year', 'month', '"projects"."id"')
  end

  private

  attr_reader :relation

  def select_monthly_data_and_dates
    <<~SQL.squish
      "projects"."id" AS "project_id",
      SUM("usages"."usage") AS "monthly_usage",
      date_part('year', "usages"."timestamp")::INT AS "year",
      date_part('month', "usages"."timestamp")::INT AS "month"
    SQL
  end
end

MonthlyUsage.all
MonthlyUsage.new(relation: Project.where(name: 'Stackoverflow')).all

https://medium.com/selleo/essential-rubyonrails-patterns-part-2-query-objects-4b253f4f4539
https://medium.flatstack.com/query-object-in-ruby-on-rails-56ea434365f0


Partitioning strings into substrings of fixed length I've got a Clojure function here that's meant to parse a string of form:

  "DDXXXYYY"

where DD is meant to be discarded, and XXX and YYY are string representations of integers.

(defn split-id [tileid]
  (map #(Integer/parseInt %)
       (map (partial apply str)
            (partition 3 (drop 2 tileid)))))

Or, written with the threading macro:

(defn split-id [tileid]
  (map #(Integer/parseInt %)
       (map (partial apply str)
            (->> tileid (drop 2) (partition 3)))))

Does anyone have any recommendations for a simple way to do this? It feels like clojure.contrib.string should have a partition function that takes a string and an integer.

I think I've come up with a fairly good block of code for the above function, which I had to extend a bit. I'd love comments and suggestions on the final block.

(defn parse-ints
  "Converts all strings in the supplied collection to their integer
  representation."
  [coll]
  (map #(Integer/parseInt %) coll))

(def res-map
  ^{:doc "Map between the second digit in a MODIS TileID
metadata value and the corresponding resolution."}
  {:1 "1000"
   :2 "500"
   :4 "250"})

(defn tileid->res
  "Returns a string representation of the resolution referenced by the
supplied MODIS TileID."
  [tileid]
  (res-map (keyword (subs tileid 1 2))))

(defn tileid->xy
  "Extracts integer representations of the MODIS X and Y coordinates
referenced by the supplied MODIS TileID."
  [tileid]
  (parse-ints
   (map (partial apply str)
        (partition 3 (subs tileid 2)))))

(defn split-id
  "Returns a sequence containing the resolution, X and Y
  coordinates (on the MODIS grid) referenced by the supplied MODIS
  TileID."
  [tileid]
  (flatten
   ((juxt tileid->res
          tileid->xy) tileid)))

Alternative solution is to keep it really simple:

(defn extract-int [s start end]
  (Integer/parseInt (subs s start end)))

(defn split-id [s]
  (list
    (extract-int s 2 5)
    (extract-int s 5 8)))

I believe that hardcoding two calls to extract-int is a simpler and more elegant way to meet the requirement than a solution which involves a map. Sure it's nice to use abstraction with higher-order functions, but YAGNI principle applies.


Multiplying an array by a number in C and how can achive vectorize the principal loop in this function? I have code in C that multiplies each element of an array by a number (0-9), resulting in a series of base 10 digits.

I compile with

gcc -xc -Ofast -msse2 -flax-vector-conversions -ffast-math -funroll-all-loops --param max-unroll-times=50 -ftree-vectorize -fopt-info-vec-missed

My processor is core i7 950.
My problem is that this function takes longer to run that I expected (8 seconds in my faster version). I need it to be faster.

I know that my problem is that
can't vectorize the principal loop for because of this error: not vectorized: not suitable for gather load x_60 = table[_59]. How can this code be modified to solve this problem and make the code faster? It's fine for the solution to use intrinsics or other specialized techniques.

Compilable code with different tests here.

My fastest version so far is this:

  uint8_t ConstMul(uint8_t *V, size_t N, uint8_t digit){
        #define TABLE_SIZE ((9*256 + 9)*9 + 9 + 1)
        static uint32_t table[TABLE_SIZE];
        if(!table[1]){
                #pragma simd
                for(uint32_t i = 0; i < TABLE_SIZE; ++i){
                        uint32_t u = i % 256 % 10;
                        uint32_t d = (i / 256 + i % 256 / 10) % 10;
                        uint32_t c = (i / 256 + i % 256 / 10) / 10;
                        table[i] = c | (u << 8)|(d << 16);
                }
        }
        if(N == 0 || digit <= 1){
                if(digit == 0) memset(V,0,N);
                return 0;
        }else{
                size_t CARRY = 0;
                if((uintptr_t)V & 1){
                        int R = V[0] * digit + (uint8_t)CARRY;
                        CARRY = (uint8_t)(R / 10);
                        V[0] = (uint8_t)(R - CARRY * 10);
                        V++;
                        N--;
                }
                {
                        uint16_t *V2 = (uint16_t *)(void *)V;
                        size_t N2 = N / 2;
                        for(size_t i = 0; i < N2; ++i){
                                uint32_t x = table[V2[i] * digit + CARRY];
                                V2[i] = (uint16_t)(x >> 8);
                                CARRY = (uint8_t)x;
                        }
                }
                if(N & 1){
                        int R = V[N-1]*digit + (uint8_t)CARRY;
                        CARRY = (uint8_t)(R/10);
                        V[N-1] = (uint8_t)(R - CARRY * 10);
                }
                return (uint8_t)CARRY;
        }
        #undef TABLE_SIZE
}

But I also tried these approaches which were slower:

void ConstMult( uint8_t *V, size_t N, uint8_t digit )
    {
      uint8_t CARRY = 0;
      for ( size_t i=0; i< N; ++i )
      {
        V[i] = V[i] * digit + CARRY;
        CARRY = ((uint32_t)V[i] * (uint32_t)0xCCCD) >> 19;
        V[i] -= (CARRY << 3) + (CARRY << 1);
      }
    }
uint8_t ConstMult( uint8_t *V, size_t N, uint8_t digit )
{
  uint8_t CARRY = 0;
  for ( int i=0; i< N; i++ )
  {
    char R = V[i] * digit + CARRY;
    CARRY = R / 10;
    R = R - CARRY*10;
    V[i] = R;
  }
  return CARRY; // may be from 0 to 9
}
uint8_t ConstMult(uint8_t *V, size_t N, uint8_t digit)
{
  uint8_t CARRY = 0;
  uint8_t ja = 0;
  for (size_t i = 0; i < N; ++i) {
    uint8_t aux = V[i] * digit;
    uint8_t R = aux + CARRY;
    CARRY = ((u_int32_t)R*(u_int32_t)0xCCCD) >> 19;
    ja = (CARRY << 3) + 2*CARRY;
    R -= ja;
    V[i] = R;
  }
  return CARRY;
}

Review of your Godbolt-fu: https://godbolt.org/z/doD3Ld

You can change the language from "C++" to "C" via the dropdown in the upper right corner of the source-code-editor pane.
You wanted to be using "gcc (trunk)", not "gcc (modules)", anyway.

The biggest contributor to running time must be that uint8_t digit is being provided as a runtime parameter instead of a compile-time parameter. But your benchmark only ever calls LongNumConstMult with 9, 8, 7, and 3. You should benchmark what happens if you write four different versions of the code: one with static const int digit = 9; at the top, one with static const int digit = 8;, and so on. Maybe that won't meet your design requirements, but it will give you a nice bound on what kind of improvement might be possible.

I infer that maybe you only need to handle 10 different digits. In that case, you could implement the runtime-parameterized LongNumConstMult as

void LongNumConstMult(uint8_t *V, size_t N, uint8_t digit)
{
    switch (digit) {
        case 0: return LongNumSetTo0(V, N);
        case 1: return;  // no-op
        case 2: return LongNumConstMult2(V, N);
        case 3: return LongNumConstMult3(V, N);
        [...]
        case 8: return LongNumConstMult8(V, N);
        case 9: return LongNumConstMult9(V, N);
    }
}

I predict that "one branch at the beginning, followed by many constant multiplications in a loop" might well beat "many non-constant multiplications in a loop."

  It's fine for the solution to use intrinsics or other specialized techniques

What about making V an array of uint16_t, uint32_t, uint64_t, or even __uint128_t? Even if V remains uint8_t, could you type-pun it to load 8 or 16 bytes at a time and do the multiplication at that width? (What is the native width of your machine?)

Here's some code that's in C++, so not directly applicable to your case, but you might find it useful: https://quuxplusone.github.io/blog/2020/02/13/wide-integer-proof-of-concept/
The code itself uses some x86 compiler intrinsics that may be relevant to your interests.


Check non-continuous range for equal values Background

I'm trying to check equality between cells in an irregular, non-continuous range of cells. For this purpose I would like to know if all cells are equal to the first Cell in the Range. The Range can be anything, but let's say it could be the following (don't mind the implicit reference for now):

Range("A1:C1,F1,K1,X1,AA1")

Sample Code:

Because the Range is non-continuous, there is no way to use the COUNTIF as a WorksheetFunction, nor can I implement some sort of INDIRECT within Evaluate. So I was looking for the easiest way to do this. I came up with the following:

'Get a 1D-array from columns of interest
Dim arr As Variant: arr = Application.Index(Range("A1:AA1").Value, 1, Array(1, 2, 3, 6, 11, 24, 27))

'Join the 1D-array and check against 7* the value of A1
If Join(arr, "") = Replace("???????", "?", Range("A1").Value) Then
    Debug.Print "All equal to first cell"
Else
    Debug.Print "Not all equal to first cell"
End If

Question:

I've spent quite some time trying to come up with some code that would avoid iteration over the Range object's Cells (or iteration of any type), for example like below:

'Get dictionary ready
Dim dict As Object: Set dict = CreateObject("Scripting.Dictionary")

'Get range of cells
Dim rng As Range: Set rng = Range("A1:C1,F1,K1,X1,AA1")

'Iterate over cells
For Each cl In rng
    dict(cl.Value) = 1
Next cl

'Test for same values
If dict.Count = 1 Then
    Debug.Print "All equal to first cell"
Else
    Debug.Print "Not all equal to first cell"
End If

It has also become clear to me that there is no simple COUNTIF of an Array. And also something like Filter has proven to not be "error" proof.

While this is working code, I was hoping to find out the best way to know if all values in an irregular, non-continuous range of cells are the same. And therefor hope CR is the right place to ask.

Using a Dictionary to count the occurences of a single value is overkill.  If the question was count the number of values that match the first then If FirstValue = cl.Value Then Count = Count + 1 would be far more efficient than using a Dictionary.

  I would like to know if all cells are equal to the first Cell in the Range

It seems to me that you are over complicating things.  Based on the objective above we are looking for a simple True or False answer.  Currently, you have to iterate over all the cells before you can determine whether all the cell value match. So it takes the same amount of time whether all cells match or not.  It would be far better to set a flag and exit the loop when a unmatched value is found.

'Get range of cells
Dim rng As Range
Set rng = Range("A1:C1,F1,K1,X1,AA1")

Dim Value As Variant
Value = rng.Cells(1, 1).Value
Dim flag As Boolean

For Each cl In rng
    If cl.Value <> Value Then
        flag = True
        Exit For
    End If
Next cl

'Test for same values
If Not flag Then
    Debug.Print "All equal to first cell"
Else
    Debug.Print "Not all equal to first cell"
End If


Simple multithreading C project I'm fairly new to C, and this is my first time using pthread. I am pretty sure I implemented it correctly, however I'd like to verify it is indeed executing the tasks parallel instead of sequentially. Here is my main.c code:

main.c

#include "frequency.h"

/**
 * The main function. Runs {@code getMedianWord()} in parallel threads for each file,
 * then outputs the sorted elements in FileArray file_array.
 */
int main(int argc, char **argv) {
    int actual_args = argc - 1;
    pthread_t tid[argc];

    for (int i = 0; i < actual_args; ++i)
        pthread_create(&tid[i], NULL, getMedianWord, (void *) &argv[i + 1]);

    for (int j = 0; j < actual_args; ++j)
        pthread_join(tid[j], NULL);

    qsort(file_array,
          (size_t) actual_args,
          sizeof(FileArray),
          (int (*)(const void *, const void *)) file_cmp
    );

    for (int k = 0; k < actual_args; ++k)
        printf("\n%s %d %s",
               file_array[k].filename,
               file_array[k].num,
               file_array[k].median_word
        );

    pthread_exit(NULL);
}

frequency.c

...
void *getMedianWord(void *vargp) {
    char *filename = *(char **) vargp;
    WordArray word_array[MAX_WORDS];

    FILE *fp = fopen(filename, "r");
    char word[101];

    int n = 0;
    while (!feof(fp)) {
        fscanf(fp, "%s", word);
        insert_word(word_array, &n, word);
    }

    insert_arr(file_array, filename, n, word_array);

    pthread_exit(NULL);
}

The intended code to be executed in parallel is the function getMedianWord, which adds a result to a global array of structs (file_array), and terminates via pthread_exit(NULL).

Please let me know if this is actually using threads properly, thanks!

PS. If you're interested in seeing the rest of my code, here it is.

I agree with commenter @vnp, who says, "The question would greatly benefit from adding getMedianWord code directly. As posted, it is on the verge of being closed as hypothetical." All the interesting stuff is going on in getMedianWord.

TLDR: yep, your use of pthread_create and pthread_join looks fine.

However, I couldn't be sure of that without knowing the declaration of getMedianWord, for which I had to click through to frequency.c:

void *getMedianWord(void *vargp) {
    char *filename = *(char **) vargp;

Okay, this is fine.

However, you could do better by passing the char pointer itself as your void* argument, instead of passing the address of the pointer. That is, if you rewrote your getMedianWord function —

void *getMedianWord(void *vargp) {
    const char *filename = vargp;

— then you could rewrite your main loop correspondingly —

for (int i = 1; i < argc; ++i) {
    pthread_create(&tid[i], NULL, getMedianWord, argv[i]);
}

Notice that I've made two other cosmetic changes here. First, I put braces around every compound statement; see goto fail for why. Second, I took your complicated actual_args/i + 1 logic and turned it into an idiomatic loop running over the half-open range [1, argc). Use the most common idioms you can! It saves your reader some brain cells.

The functional change here is replacing &argv[...] with simply argv[...]. (The cast to void* is not required in C. And likewise the cast back from void* inside getMedianWord is not required in C, although it would be required if you wanted to port this code to C++.)

I won't very closely review the rest of getMedianWord because you didn't post it.

It is definitely not thread-safe, though; there's a data race on file_index. Look up the _Atomic keyword.

I notice that getMedianWord spends most of its time reading from files, which means that it's not likely to parallelize very well. You'll end up being bounded by the speed of your file-reading. Splitting up those reads into different threads doesn't achieve any speedup; in fact it probably wastes time because now you have to wait for a context-switch between each pair of reads. (OTOH, the files will be buffered at the stdio level and also probably brought into memory at the OS level, so maybe I'm wrong about its hurting. I think I'm right about its not helping, though.)

Try reading all the files into memory first, and then splitting up just the computational processing of those files into multiple threads.

In other words, think of "the filesystem" as a contended resource which will destroy your parallelism if everyone's trying to access it at once. (Also think of the malloc/free heap and stdout as contended resources. Looks like you're doing well on those fronts.)

Your WordArray word_array[MAX_WORDS]; is a variable-length array, which might blow up your (thread's) stack if MAX_WORDS is increased to, say, 2000000 / sizeof(WordArray) — that is, if MAX_WORDS * MAX_STRING >= 2000000. So watch out for that.


Filtering users by criteria in Ruby on Rails I am a fairly new (RubyonRails) developer. I desire to improve my coding skills so I used climate to do some review on my code. It gave me a flag for this method that its complex. Is it characterised as complex because of having several "actions/tasks" in a single method?

Will it be better if I extract some code segments to a different method?

Is there something else I am not seeing?

  def search
    filter_mapping = {"cost" => "rcost", "quality" => "rquality", "time" => "rtime", "experience" => "rexperience", "communication" => "rcommunication"}
    @filters = params[:filter].split(",") rescue []
    @sort = params[:sort]

    @user_type = params[:s_user_type]
    @skills = params[:s_skills]
    @location = params[:location]
    @max_rate = params[:max_rate]
    @availability = params[:availability]

    @users = User.scoped.joins(:user_skills)
    @users = @users.where('user_type = ?', @user_type) if @user_type.present?
    @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0
    @users = @users.where('availability = ?', @availability) if @availability.present?
    @users = @users.where('location_country = ?', @location) if @location.present?
    @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?
    @users = @users.page(params[:page]).per(PER_PAGE)

    @filters.each do |f|
      if filter_mapping.keys.include?(f)
        @users = @users.order("#{filter_mapping[f]} desc")
      end
    end

    @users = @users.order('id asc') if @filters.empty?
    @advanced_link = @location.blank? && @max_rate.blank? && @availability.blank?
    render :index
  end

Update

I figured out that I can extract the scopes into a method like that:

  def get_users_where_scopes
    @users = User.scoped.joins(:user_skills)
    @users = @users.where('user_type = ?', @user_type) if @user_type.present?
    @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0
    @users = @users.where('availability = ?', @availability) if @availability.present?
    @users = @users.where('location_country = ?', @location) if @location.present?
    @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?
    @users = @users.page(params[:page]).per(PER_PAGE)
  end

and then call it with @users = get_users_where_scopes(). But now the complexity of this method seems wrong to me.

I'd say to first make a service object to keep the controller lean and clean, and to give yourself a place to put all the logic without fear of polluting the controller. Plus: It's reusable!

# app/services/user_search.rb

class UserSearch
  ORDER_MAPPING = {
    "cost"          => "rcost",
    "quality"       => "rquality",
    "time"          => "rtime",
    "experience"    => "rexperience",
    "communication" => "rcommunication"
  }.freeze

  def initialize(params)
    @params = params
  end

  def results
    @results ||= begin
      records = User.scoped.joins(:user_skills)
      records = scope(records)
      records = order(records)
    end
  end

  private

  def param(key)
    @params[key] if @params[key].present?
  end

  def scope(scoped)
    scoped = add_scope(scoped, 'user_type = ?', param(:user_type))
    scoped = add_scope(scoped, 'user_skills.skill_id in (?)', skill_ids)
    scoped = add_scope(scoped, 'availability = ?', param(:availability))
    scoped = add_scope(scoped, 'location_country = ?', param(:location))
    scoped = add_scope(scoped, 'rate < ?', max_rate)
  end

  def add_scope(scope, sql, *params)
    scope.where(sql, *params) if params.all?(&:present?)
    scope
  end

  def order(scope)
    terms = sanitized_order_terms || default_order_terms
    terms.each { |term| scope.order(term) }
    scope
  end

  def sanitized_order_terms
    terms = param(:filter).try(:split, ",")
    terms = terms.map { |term| ORDER_MAPPING[term] }
    terms = terms.compact
    terms if terms.any?
  end

  def default_order_terms
    ["id asc"]
  end

  def skill_ids
    param(:s_skills).try(:map, &:to_i)
  end

  def max_rate
    param(:max_rate).try(:to_i)
  end
end

I've intentionally kept the pagination in the controller, as it's pretty independent of the scoping and ordering. However, it'd be simple to add as arguments to the #results method

In your controller:

def search
  @users = UserSearch.new(params).results.page(params[:page]).per(PER_PAGE)

  advanced_params = %w(location max_rate availability).map { |p| params[p] }
  @advanced_link = advanced_params.all?(&:blank)

  render :index
end

I'd probably pick a more direct way of determining the @advanced_link, such as sending an advanced parameter along, and simply looking at that instead of the implicit state you have now.

I have no idea what Code Climate thinks of the code above, but I imagine it'll be happier.


Null check in .equals implementation I have inherited some legacy code that I'm now trying to clean up.

I feel like the 2nd null check is redundant, see comments:

private java.lang.Object __equalsCalc = null;
    public synchronized boolean equals(java.lang.Object obj) {
        if (!(obj instanceof ArrayOfColumn)) return false; // Here is first check if the obj is not null, if there is instaceof ArrayOfColumn then it's not null
        ArrayOfColumn other = (ArrayOfColumn) obj;
        if (obj == null) return false; // Therefore this check is redundant and "return false" will never execute, so it's dead code
        if (this == obj) return true;
        if (__equalsCalc != null) {
            return (__equalsCalc == obj);
        }
        __equalsCalc = obj;
        boolean _equals;
        _equals = true &&
            ((this.column==null && other.getColumn()==null) ||
             (this.column!=null &&
              java.util.Arrays.equals(this.column, other.getColumn())));
        __equalsCalc = null;
        return _equals;
    }

Anything else?

First up, yes, you're right that the second null check is redundant. If obj is null then the method will return false on the first check:

    if (!(obj instanceof ArrayOfColumn)) return false;

Of course, that would better be written:

    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }

But, that's the least of my concerns. The synchronized equals is probably there because other methods are synchronized, but synchronization comes at a cost. Unless you are sure you need it, remove it.

Additionally, synchronized methods are seldom the best solution. It is normally better to have tighter control of your locks so that nobody but you can hold them. That normally means using a private, internal "monitor" for synchronization:

private final Objct lock = new Object();

private boolean equals(Object obj) {
    synchronized(lock) {
        .....
    }
}

Using the private lock prevents other people from hanging your application by using your class as their own monitor. Imagine if your class is called MyClass, and someone does:

 private MyClass instance = new MyClass();

 synchronized(instance) {
     Thread.sleep(10000);
 }

If they did the above, your methods in other threads would never run, and your application would "hang".

Now, abut the __equalsCalc variable. It does nothing useful. It is set, and then cleared, inside the synchronized method, and, as a result, it will never, ever have any value in it that is useful. It is completely redundant and a waste of time.

Oh, and what's with the fully qualified java.lang.Object? Just use Object.

I would rewrite the method as (using the same synchronized method to be compatible with other methods in your class):

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    if (getColumn() == null) {
        return other.getColumn() == null;
    }
    return other.getColumn() != null && Arrays.equals(column, other.getColumn());
}

Note that you still have a synchronization vulnerability - the other column's column could be changed between checking for whether it's column is null, and using the column in the equals. You could still get null pointer exceptions if someone changes the other column in the middle of your equals. You should consider synchronizing on that other column too:

public synchronized boolean equals(Object obj) {
    if (!(obj instanceof ArrayOfColumn)) {
        return false;
    }
    if (obj == this) {
        return true;
    }
    ArrayOfColumn other = (ArrayOfColumn) obj;
    synchronized(other) {
        if (getColumn() == null) {
            return other.getColumn() == null;
        }
        return other.getColumn() != null && Arrays.equals(column, other.getColumn());
    }
}

Unfortunately, cross-synchronizing like that can lead to deadlocks if you are not careful.


Disabling a form button while there's an http request running I have a form built with react, formik and yup, and this form has some async validations. Apparently, some users were able to bypass these validations, and to prevent some errors, I wanted to disable the submit button when there's a pending http request.

Many years ago, I used to handle this very easily with jQuery, but now, this is not the case.

I came up with a solution where I used the useEffect hook to set a state in case there's an http request running, and the way I'm detecting this is what I wanted your opinion.

Basically:

import React, { useEffect, useState } from 'react';
import axios from 'axios';
import { Input } from '../../../../components/Formik';

const Form = props => {
  const [disableSubmitBtn, setDisableSubmitBtn] = useState(false);

  useEffect(() => {
    axios.interceptors.request.use(
      configs => {
        console.log('http req running');
        setDisableSubmitBtn(true);
        return configs;
      },
      error => {
        return Promise.reject(error);
      },
    );
    console.log('no http req running');
    setDisableSubmitBtn(false);
  }, [disableSubmitBtn]);

  return (
    <form
      className="form-horizontal"
      onSubmit={e => {
        props.handleSubmit(e);
      }}
    >
      {props.settings.bonuscode && (
        <fieldset>
          <Input
            id="marketing.bonus_code"
            {...props.user.marketing.bonus_code}
            {...props}
            classNameLabel="col-md-5"
            classNameInput="col-md-7"
          />
        </fieldset>
      )}

      <button
        id="wordreg-form-submit"
        type="submit"
        disabled={props.settings.formSubmited || disableSubmitBtn}
      >
        Submit
      </button>
    </form>
  );
};

export default Form;

So what matters is actually the code inside the useEffect hook. Whad do you think about it?
Is it ok to check for pending http requests this way?

Yes, your approach looks correct to me. I also had faced similar issues earlier in my project (before hooks were introduced) and had used setState for a boolean variable to enable and disable the button.

The thing which you need to keep in mind is look out for edge cases related to your business requirements. e.g. suppose a async validation is happening but API gave you 401 or some other status code (i.e. validation didn't happen properly but API gave some response), so should you be enabling the button for user to retry or the button should remain disable.

Hoping that I am able to explain you properly. Please revert if something is not clear to you.


Weighted union-find with path compression in Python 3 I implemented the Union-Find data structure and would love to receive some feedback about my code. I have just begun programming and welcome any feedback or constructive criticism which would improve my code quality and professionalism.

class UnionFind:

    def __init__(self, N=1001):
        self.N = N
        self.id = [i for i in range(self.N)]
        self._rank = [1] * self.N

    def __root(self, x):
        while x != self.id[self.id[x]]:
            self.id[x] = self.id[self.id[x]]
            x = self.id[x]
        return x

    def find(self, p, q):
        return self.__root(p) == self.__root(q)

    def unite(self, p, q):
        i = self.__root(p)
        j = self.__root(q)
        if self._rank[j] > self._rank[i]:
            i, j = j, i
        self.id[j] = i
        self._rank[i] += self._rank[j]

I was working on the problem of finding cycles in undirected graphs and realised union-find could be used to efficiently solve this problem and hence, implemented it.

In this instance the input would be a list of [u, v] pairs where u and v are integers and u < v. u and v are the node numbers of the graph under consideration.

For example the input [[1, 3], [2, 3], [1, 2]] would represent

Finding cycles

The idea is as follows

  iterate over the [u, v] pairs
  if find(u, v): there exists a cycle since [u, v] belong to the same connected component
  else: unite(u, v)

For the example graph [[1, 3], [2, 3], [1, 2]] above, the code would return on [1, 2] since adding this would result in a cycle.

unite should check if p and q are in the same set already and do nothing in that case.
Using dictionaries instead of lists as internal data structures would make this more flexible. Now the elements must be from range(N) and N has to be known at the start. Wikipedia suggests a MakeSet function which you could implement like this, and your other functions would continue to work unchanged:

def __init__(self):
    self.id = {}
    self._rank = {}

def make_set(self, element):
    self.id[element] = element
    self._rank[element] = 1

id is a non-obvious name for a data structure stores the parent of each node in a tree. Why not parent?
A single leading underscore is conventionally used to indicate private data.
It is possible condense these two lines to one and avoid the extra lookup of self.id[x]

self.id[x] = self.id[self.id[x]]
x = self.id[x]

with a chained assignment. However, you risk confusing a reader who is not well familiar with the order of evaluation. The right hand side is evaluated first, and the assignments are carried out starting from self.id[x] on the left, and the value of  x changes after:

self.id[x] = x = self.id[self.id[x]]