Are these file handling functions safe? I'm using these 2 functions to handle file input/output and would like to know if there's anything that should be changed.

For content retrieval, enough space is allocated and the file content is copied. To make sure saving a file either saves all or nothing, I'm writing to a buffer then renaming it after all content is successfully written.

I'm particularly interested in knowing whether these functions are safe or not since tmpnam is considered dangerous. I read the main risk is a file with the name returned by tmpnam being created before we are able to do it, so I'm using wbxto make sure it doesn't happen. Is this enough or there are other security concerns that I'm not aware of?

files.h

#ifndef FILES_H
#define FILES_H

#include <stdlib.h>

#define FILE_SUCCESS 0
#define FILE_ERROR 1

void *file_get_content(const char *file_name, size_t *file_size_fill);

//Save all or nothing
int file_save_content(  const char *file_name,
                        const void *content,
                        size_t size             );

#endif

files.c

#include <stdio.h>
#include <sys/stat.h>
#include "files.h"

#define BYTE 1
#define READ_MODE "rb"

//Use the new "x" subspecifier so fopen will fail if a file with the name
//returned by tmpnam exists
#define SAFE_WRITE "wbx"

void *file_get_content(const char *file_name, size_t *file_size_fill)
{
    //Get file size, SEEK_END is not portable
    struct stat file_stat;
    if(stat(file_name, &file_stat) != 0)
        return NULL;

    //Check if the file actually contains anything, since malloc can return
    //values other than NULL for malloc(0)
    if(file_stat.st_size == 0)
        return NULL;

    //Space to save the contents
    void *content = malloc(file_stat.st_size);
    if(content == NULL)
        return NULL;

    //Open file
    FILE *file = fopen(file_name, READ_MODE);
    if(file == NULL){
        free(content);
        return NULL;
    }

    //Read it all
    if(fread(content, BYTE, file_stat.st_size, file) != file_stat.st_size){
        free(content);
        fclose(file);
        return NULL;
    }

    fclose(file);

    *file_size_fill = file_stat.st_size;
    return content;
}

//Save the file content safely, it either saves all or nothing
int file_save_content(  const char *file_name,
                        const void *content,
                        size_t size             )
{
    //Get a name for the temp file
    char temp_file_name[L_tmpnam];
    if(tmpnam(temp_file_name) == NULL)
        return FILE_ERROR;

    //Create it
    FILE *temp_file = fopen(temp_file_name, SAFE_WRITE);
    if(temp_file == NULL)
        return FILE_ERROR;

    //Write
    if(fwrite(content, BYTE, size, temp_file) != size){
        fclose(temp_file);
        remove(temp_file_name);
        return FILE_ERROR;
    }

    //Replace old file
    fclose(temp_file);
    if(rename(temp_file_name, file_name) != 0){
        remove(temp_file_name);
        return FILE_ERROR;
    }

    return FILE_SUCCESS;
}

file_save_content() after following the suggestions given by 200_success

int file_save_content(const char *file_name, const void *content, size_t size)
{
    /* Create temp file */
    char tmp[] = {'t','m','p','X','X','X','X','X','X', '\0'};
    int error;
    int fd = mkstemp(tmp);

    if(fd == -1)
        return FILE_ERROR;

    if(write(fd, content, size) != (ssize_t)size){
        error = errno;
        close(fd);
        errno = error;
        return FILE_ERROR;
    }

    /* Replace old file */
    if(rename(tmp, file_name) != 0){
        error = errno;
        unlink(tmp);
        close(fd);
        errno = error;
        return FILE_ERROR;
    }

    close(fd);

    return FILE_SUCCESS;
}

I think that a more idiomatic C interface would be

ssize_t file_get_content(const char *file_name, char *content[]);

Returning the number of bytes actually read would be analogous to read(2) and printf(3).  On error, the return value could be -1, and the reason would be in errno.  For consistency, I would have file_save_content() return the number of bytes written, or -1 on error.

It would be convenient if you allocated an extra byte and appended a \0 terminator to the result.  Then the caller has the option to treat the buffer as a null-terminated string, if it believes the file to contain text.  It would still be able to treat it as binary data (possibly containing NUL bytes) based on the return value.

When getting the contents of an empty file, I would expect it to produce an "empty" buffer, not a NULL pointer.

There is a possibility of a race condition: the file could be truncated between the stat() and the fread().  You could actually handle that case a bit more gracefully.

int file_get_content(const char *file_name, char *buf[]) {
    struct stat file_stat;
    if (0 != stat(file_name, &file_stat)) {
        return -1;
    }

    if (NULL == (*buf = 1 + malloc(file_stat.st_size))) {
        return -1;
    }

    FILE *file;
    if (NULL == (file = fopen(file_name, "rb"))) {
        free(*buf);
        return -1;
    }

    ssize_t bytes_read = fread(*buf, 1, file_stat.st_size, file);
    (*buf)[bytes_read] = '\0';
    fclose(file);
    return bytes_read;
}

In file_save_content(), avoid tmpnam(), which allows a race condition between the time you call tmpnam() and fopen().  Also, rename() would fail unless the temporary file and the destination are on the same filesystem.  As an alternative, I would recommend mkstemp() with the destination filename plus a ".XXXXXX" suffix, followed by fdopen().

As a nitpick, I'd prefer unlink() over remove(), since the former refuses to remove directories.

In the error handlers of file_save_content(), save and restore errno, because it would be more useful for the caller to see the original reason for the failure rather than a problem with the subsequent cleanup.


Structure pages mvc + oop I am developing a website and I want to use PDO and MVC.

Before, I was coding in procedural MySQL, but I'm starting to understand the object-oriented programming.

My problem is the following:

If I understand the MVC pattern, there are 3 parts

View (It's my Html)
Model (PHP requests)
Controller (Control and form validation)

Right?

I want to know if I have the right approach to MVC.

So, I put code that works and I try to change in MVC. (My test MVC is lower in the page.)

$db = new DbConnect();
$error = '';
if(isset($_POST['login_submit'])){
   if(empty($_POST['login']) || empty($_POST['password'])){
       $error = 'All fields required';
   }else{
       $db->query('SELECT * FROM users WHERE u_login = :login AND u_password = :password');
       $db->bind(':login', $_POST['login']);
       $db->bind(':password', md5(sha1($_POST['password'])));
       $row = $db->single();
       if($db->rowCount() > 0){
           echo 'I can save some informations by sessions';
           echo '
';
           print_r($row);
           echo '
';
       }else{
           $error .= 'Not find in the Data Base';
       }
   }
}
if(!empty($error)){
    echo $error;
}

Here the html form:

<form action="login" method="post" name="login">
Login:
    <input type="text" name="login" id="login">

Mot de passe:
    <input type="password" name="password" id="password">

<input type="submit" name="login_submit" value="Se connecter">

Now I try to evolve towards the MVC model like this:

1. My Model called model.login.php:

class Auth extends DbConnect{
    protected $login;
    protected $password;
    protected $email;

    public function ConnectSecure(){
        if(isset($_POST['login_submit'])){
            CheckPostLogin();
            $this->query('SELECT * FROM users WHERE u_login = :login AND u_password = :password');
            $this->bind(':login', $_POST['login']);
            $this->bind(':password', md5(sha1($_POST['password'])));
            $this->single();
            if($this->rowCount() > 0){
                echo 'OK and I put some informations by sessions';
            }else{
                echo 'No in DB or error combination user/password';
            }
            echo '
';
            print_r($result);
            echo '
';
        }
    }
}

2. My controller called controller.login.php:

require 'models/model.login.php';
function CheckPostLogin(){
    if(empty($_POST['login']) || empty($_POST['password'])){
        Message::ShowError('All fields required.');
        return false;
    }
}

3. My view has the same HTML code listed above.

I have also a global class that will handle static functions to display messages for error, success, etc. like this (just an example):

class Message{
    public static function ShowError($message){
        echo $message;
    }
}

I have another problem:

Inside my model, after the query, I check if I have some result. I think this part need go on the controller. Right? If yes, how can I do this properly?

I'm new to OOP. If you see any errors or discrepancies, your advice is welcome.

Well .. you do not understand MVC design pattern. You seem to got all three parts wrong. Since I am quite lazy person, I will just direct to some older posts of mine that might help you to deal with handling pdo, some initial studies for the subject, basics of views, model layer, some minor note on controllers. Also there is a bit about access control in context of MVC, that might be useful to you.

Now! To the code ...
File: model.login.php

Model is not subtype of database connection (wel .. model is not even a class). This class is violating SRP [1], [2], [3] .

Aside from making a pointless wrapper for PDO, there is also a serious problem there: md5(sha1( ... )) should NEVER be used for hashing passwords. Please read the following three posts from @ircmaxell.

File: controller.login.php

You should learn how to use autoloader. Start by reading the documentation about spl_autoload_register(). Also I would recommend to look into what are namespaces and how then can be used together with autoloading schemes.

Don't us static method calls. Just because you wrap something in a class does not make it object-oriented (notice how it isn't called "class-oriented programming" .. there is a distinction). When used this way, your class acts only as namespace for your global-scope functions.

... the rest, IMHO, is covered in the linked posts above.


Show all full/partial search matches in the first worksheet from other worksheets Search and display all instances of a full/partial match in the first worksheet from all the other worksheets in the workbook after running a macro to clear formatting of previous search results.

I have data in over 1000 cells in one workbook, spread over 10 worksheets that I am able to search through using a button.

If I use find and replace the data is found within seconds, but using the button it takes a little longer. The idea is that it is much more user friendly and reports the results into one of the worksheets and the next time you click the search button it removes the search results and all formatting.

Can someone please tell me if there is anything I can do to speed it up?

The workbook contains several worksheets with data and each worksheet has different headers. Within the search page, I would like to include the headers at the top of each list of results and then insert a blank row between the search results of each sheet on the search page but I'm not sure how this should be implemented?

Sub Set_Search()
Call Reset1
 '   Object variables
Dim wks As Excel.Worksheet
Dim rCell As Excel.Range
Dim fFirst As String
 '   {i} will act as our counter
Dim i As Long
 '   Use an input box to type in the search criteria
Dim MyVal As String
MyVal = InputBox("Enter the full name of an individual, department, organisation or area that you wish to contact.", "Contact Search", "")
 '   if we don't have anything entered, then exit the procedure
If MyVal = "" Then Exit Sub
Application.ScreenUpdating = False
Application.DisplayAlerts = False
'       Add a heading to the sheet with the specified search value
With Cells(1, 1)
.Value = "Found '" & MyVal & "' in the following cells:" & vbCrLf & "(click below to view the original data)"
.EntireColumn.AutoFit
.HorizontalAlignment = xlCenter
.VerticalAlignment = xlCenter
    With Selection.Font
         .Name = "Arial"
         .FontStyle = "Regular"
         .Size = 10
    End With
i = 2
 '       Begin looping:
 '       We are checking all the Worksheets in the Workbook
For Each wks In ActiveWorkbook.Worksheets
     If wks.Name <> "Search" Then
     '       We are checking all cells, we don't need the SpecialCells method
     '       the Find method is fast enough
        With wks.Range("A:G")
         '           Using the find method is faster:
         '           Here we are checking column "A:G" that only have {myVal} partially
            Set rCell = .Find(MyVal, , , xlPart, xlByColumns, xlNext, False)
         '           If something is found, then we keep going
            If Not rCell Is Nothing Then
             '               Store the first address
                fFirst = rCell.Address
                Do
                 '                   Link to each cell with an occurence of {MyVal}
                    rCell.Hyperlinks.Add Cells(i, 1), "", "'" & wks.Name & "'!" & rCell.Address
                    wks.Range("A" & rCell.Row & ":I" & rCell.Row).Copy Destination:=Cells(i, 2)
                    Set rCell = .FindNext(rCell)
                    i = i + 1 'Increment our counter
                Loop While Not rCell Is Nothing And rCell.Address <> fFirst
            End If
        End With
     End If
Next wks
 '   Explicitly clear memory
Set rCell = Nothing
 '   If no matches were found, let the user know
If i = 2 Then
    MsgBox "No results containing your search term were found." & vbCrLf & "" & vbCrLf & "Your search for '" & MyVal & "' did not match any exising data." & vbCrLf & "" & vbCrLf & "These contact details will be added if you send an email to me.", 64, "Contact Search"
    Cells(1, 1).Value = ""
End If
 '   Reset application settings
Application.ScreenUpdating = True
Application.DisplayAlerts = True
End Sub

The Find method is pretty quick but has some overhead due to all the find options e.g. whole cells, match case etc.

In this case it would be quicker to put the search data into an array and iterate that to find your value. e.g (written here, untested):

Function MatchCell (ws as Worksheet) as Range

Dim sourceData() as variant
Dim lastRow as long, x as long, y as long

lastRow = ws.Range("A1000000").end(xlUp).row ' you may want to adjust this if you don't know that column A will represent all used rows
sourceData = ws.range("A1:G" & lastRow).value ' again, adjust to suit

for x = 1 to ubound(sourceData) ' loop the first dimension

   for y = 1 to ubound(sourcedata, 2) ' loop the 2nd dimension

      if sourceData(x, y) = myVal then
         MatchCell = ws.Cells(x, y) ' the row is x, the column is y
         Exit Function ' escape the loop/function once the value is found
      End If

   next y

next x

End Function


Using RxJS Observables to mediate routes in nodejs I am learning to be a full stack developer and have been building out an application using Node JS as a backend. Since I have lots of experience using Angular I love RxJS. So naturally I was tempted to use RxJS for some types of tasks. Are there any down sides to using RxJS on the backend, more specifically are there any downs sides I can learn about from the following code implementation?

Im using Sequalize and Postgres. As you can see in the Invite Service I have used the Observable stream to first check if the Invite id is valid and then it checks if the recipient email is found in the database as a current user account or not. From either of the three returned values to the route I can now tell the front end what it must do next.

Other notable things:

I am using typical JWT authentication which in this case is a special user in the db since the request is being made from a web app and there is no actual logged in user.
I decided to pass the invite id and the recipient email in the headers which I know is kind of unusual.

This web app is actually just a mediator to deal with a complicated invite process which is being initialized from a React Native app.

Entry Point:

import inviteGenRouter from './routes/invite.routes';
let inviteRouter = inviteGenRouter(services);
api.use('/invite', inviteRouter);

app.use('/api', [passport.authenticate('jwt-bearer', {
  session: false,
  failWithError: true
}), (err, req, res, next) => {

  if (err) {
    Logger.error('Authentication Handshake: ', err);
    let error = {
      error: err.name,
      message: err.message
    };
    let statusCode = err.status || 500;
    return res.status(statusCode).json(error);
  }
  return next();
}], api);

Invite Routes:

router.route('/').get((req, res) => {
  if (req.headers.data) {
    return services.InviteService.authenticateInvite(req.headers.data)
      .subscribe(
        _res => {
          // Return options for front end.
          // If string the invite code was not found.
          // If null the associated email was not a user account.
          // If user the associated email was found.
        },
        _error => {
          // Some stuff when wrong...
        }
      );
  } else {
    // return some sort of error code.
  }

});

Invite Service:

class InviteService  {

  constructor(models) {
    this.models = models
  }

  findInviteById(id) {
    return Observable.defer(() => {
      return Observable.fromPromise(this.models.Invite.findOne({
        where: { inviteId: id }
      }));
    });
  }

  findUserById(email) {
    return Observable.defer(() => {
      return Observable.fromPromise(this.models.User.findOne({
        where: { email: email }
      }));
    });
  }

  authenticateInvite(token) {
    let payload = jwt.verify(token, appConfig.SESSION_SECRET);

    return this.findInviteById(payload.inviteId)
      .map(_invite => _invite ? _invite : Observable.of('Invalid Invite ID'))
      .mergeMap(_invite => _invite._isScalar ? _invite : this.findUserById(payload.email));
  }
}

module.exports = InviteService;

Are there any down sides to using RxJS on the backend

The way I see it, you get 2 benefits from RxJS

Reactivity
Streaming operations

Observables are effectively a form of event management. They run, listen to things, and execute code when something happens. They're a good way to respond when something happens in the app. However, when used in routing, your observable only lives during the duration of the request-response cycle. You lose all the benefits of a persistent event system when they're short-lived.

This leaves you with streaming operations which isn't something regular functions, promises, array methods and ternaries can't do. When observables are used this way, they effectively become what I call "fat promises". You're better off fully utilizing native constructs than using advanced constructs suboptimally.

Speaking of Node, async and await are available too, which makes life a bit easier:

// Routes
router.route('/').get(async (req, res) => {
  try {
    if (req.headers.data) {
      const res = await services.InviteService.authenticateInvite(req.headers.data)
      // respond with data
    } else {
      // respond with no header data error
    }
  } catch (e){
    // Something else went wrong
  }

});

// Service
class InviteService  {

  constructor(models) {
    this.models = models
  }

  async findInviteById(id) {
    return this.models.Invite.findOne({ where: { inviteId: id } })
  }

  async findUserById(email) {
    return this.models.User.findOne({ where: { email: email } })
  }

  async authenticateInvite(token) {
    const payload = jwt.verify(token, appConfig.SESSION_SECRET)
    const invite = await this.findInviteById(payload.inviteId)

    if(!invite) throw new Error('Invalid Invite ID')

    return invite._isScalar ? invite : this.findUserById(payload.email)
  }
}

module.exports = InviteService;

Also, if you end up doing more comparisons after getting authenticateInvite's return value, that means authenticateInvite is not abstracting correctly. I'd expect that after I call authenticateInvite, it should be boolean. It either resolves (auth passed) or rejects (auth failed) with the error object containing the reason for failure. All business logic (and errors) should go to the service class.


Simple hashing code with excessive conditional statements Any thoughts on my code? It's a simple hashing code that runs from the command line.

There just appears to be a lot of condition if / elif entries in both of the main functions (hashingmethod and hashcalculator).

# hashcalculator.py

import sys
import hashlib

args = len(sys.argv)

def inputVerification():
    if sys.argv[1] == "--help":
        scriptHelp()
    elif args < 5:
        print "You have failed to execute the command correctly"
        print "Please type 'hashcalculator --help' for more information"
    else:
        filename = sys.argv[2]
        hashingMethod(filename)

def scriptHelp():
    print "\n"
    print "----------------------------------------"
    print "\tWelcome to Hash Calculator"
    print "----------------------------------------"
    print "\n"
    print "--help"
    print "\n"
    print "In order to calculate the hash value of a file, your command line syntax"
    print "must be written as below:-"
    print "\n"
    print "\thashcalculator -f [file] -h [hash method]"
    print "\teg. hashcalculator -f notepad.exe -h SHA1"
    print "\n"
    print "The recognised hashing methods are MD5, SHA1, SHA224, SHA256, SHA384 and SHA512"

def hashingMethod(filename):
    hashmethod = sys.argv[4].upper()
    if hashmethod == "MD5":
        hashCalculator(filename, "md5")
    elif hashmethod == "SHA1":
        hashCalculator(filename, "sha1")
    elif hashmethod == "SHA224":
        hashCalculator(filename, "sha224")
    elif hashmethod == "SHA256":
        hashCalculator(filename, "sha256")
    elif hashmethod == "SHA384":
        hashCalculator(filename, "sha384")
    elif hashmethod == "SHA512":
        hashCalculator(filename, "sha512")
    else:
        print "You have not entered a valid hashing method"
        print "Please review the help document"
        scriptHelp()

def hashCalculator(filename, hashmethod):
    with open(filename, 'rb') as f:
        if hashmethod == "md5":
            m = hashlib.md5()
        elif hashmethod == "sha1":
            m = hashlib.sha1()
        elif hashmethod == "sha224":
            m = hashlib.sha224()
        elif hashmethod == "sha256":
            m = hashlib.sha256()
        elif hashmethod == "sha384":
            m = hashlib.sha384()
        elif hashmethod == "sha512":
            m = hashlib.sha512()
        else:
            scriptHelp()
        while True:
            data = f.read(8192)
            if not data:
                break
            m.update(data)
        print "\n\n"
        print "The %s hash of file %s is:" % (hashmethod.upper(), filename)
        print "\n", m.hexdigest()

if __name__ == "__main__":
    inputVerification()

You should really take a look at argparse: the argparse module makes it easy to write user-friendly command-line interfaces. It will even take care of writing the help for you.

Even if you will apply very little of the following (since you should really use the argparse module), I'll give you some advice anyway.

str.lower()

This:

if hashmethod == "MD5":
    hashCalculator(filename, "md5")
elif hashmethod == "SHA1":
    hashCalculator(filename, "sha1")
elif hashmethod == "SHA224":
    hashCalculator(filename, "sha224")
elif hashmethod == "SHA256":
    hashCalculator(filename, "sha256")
elif hashmethod == "SHA384":
    hashCalculator(filename, "sha384")
elif hashmethod == "SHA512":
    hashCalculator(filename, "sha512")

could just be:

hashCalculator(filename, hashmethod.lower())

Coding style

Follow the official coding style, the main rules are summarized here.

getattr()

This:

if hashmethod == "md5":
    m = hashlib.md5()
elif hashmethod == "sha1":
    m = hashlib.sha1()
elif hashmethod == "sha224":
    m = hashlib.sha224()
elif hashmethod == "sha256":
    m = hashlib.sha256()
elif hashmethod == "sha384":
    m = hashlib.sha384()
elif hashmethod == "sha512":
    m = hashlib.sha512()

could just be:

m = getattr(hashlib, hashmethod)

Multiple line strings

Don't do this:

print "\n"
print "----------------------------------------"
print "\tWelcome to Hash Calculator"
print "----------------------------------------"
print "\n"
print "--help"
print "\n"
print "In order to calculate the hash value of a file, your command line syntax"
print "must be written as below:-"
print "\n"
print "\thashcalculator -f [file] -h [hash method]"
print "\teg. hashcalculator -f notepad.exe -h SHA1"
print "\n"
print "The recognised hashing methods are MD5, SHA1, SHA224, SHA256, SHA384 and SHA512"

Do this instead:

msg = """
----------------------------------------
\tWelcome to Hash Calculator
----------------------------------------

--help

In order to calculate the hash value of a file, your command line syntax
must be written as below:-
\thashcalculator -f [file] -h [hash method]
\teg. hashcalculator -f notepad.exe -h SHA1

The recognised hashing methods are MD5, SHA1, SHA224, SHA256, SHA384 and SHA512"""

print msg


Exclude scores when flag is set I am not sure if I am just having a brain lapse here or if there is a better way to do this:

I am trying to only add one to count in the following scenarios:

the bool excludezeros is set to false (count should be increased for iterations of the foreach)
the bool excludezeros is set to true and studentScore is not equal to 0 (count only increases when student score is not equal to 0)

Here is what I have:

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;
    if (excludeZeroes && studentScore == 0)
    {
        continue;
    }
    else
    {
        count++;
    }
}

I agree with forsvarir's answer, that you should lose the continue by inverting the condition.  But you can do one better than that: lose the loop!

var scores = students.Select(student => GetScore(student));
int count = scores.Count(score => !excludeZeros || score != 0);
int total = scores.Sum();

Done. No loop needed.

Or equivalently:

var scores = from student in students
             let score = GetScore(student)
             where !excludeZeros || score != 0
             select score;
int count = scores.Count();
int total = scores.Sum();

You know what is really nice about doing it this way? First, the code looks like what it means; it's closer to the "business domain" of the code. Second, it is super easy to extract more statistics:

int maximum = scores.Max();
int average = scores.Average();
var sorted = scores.OrderBy(x=>x);
var bottomTen = sorted.Take(10);

and so on.


Range of ordinal numbers My disgusting list comprehension in the return statement is quite a headful.

"""Hopefully this function will save you the trip to oocalc/excel.

"""

def rangeth(start, stop=None, skip=1):
    """rangeth([start,] stop[, skip])

returns a list of strings as places in a list (1st, 2nd, etc)

>>> rangeth(4)
['0th', '1st', '2nd', '3rd']

    """

    if stop is None:
        stop, start = start, 0

    places = {'1':'st', '2':'nd', '3':'rd'}

    return ["{}{}".format(i, places.get(i[-1], 'th')) \
            if i[-2:] not in ['11', '12', '13'] else "{}{}".format(i, 'th') \
            for i in map(str, range(start, stop, skip))]

Also, can someone explain to me how range accepts it's parameters? I have my ugly little boilerplate here that I wish didn't exist. I can't find the source for range, as I gave up after thinking it's probably some header file in include.

Firstly, I'd suggest not duplicating the overloading logic of range. Instead:

def rangeth(*args):
    print range(*args)

Just take your arguments and pass them onto to range.

Secondly, I'd write a function that takes a single number and produces the "th"ified version.

def as_th(number):
    number = str(number)
    if number[-2:] in ['11','12','13]:
         return "{}{}".format(number, 'th')
    else:
         return "{}{}".format(number, places.get(number[-1], 'th'))

I think that makes it easier to follow.

Then have you rangeth function be:

def rangeth(*args):
    return map(as_th, range(*args))


A regex in Java. Latin letters, digits, dots, and minus signs There is a user login, and the requirements are the following:

The login must start with a Latin letter.
The login must finish with either a Latin letter or a digit.
There may also be digits, dots, and minus signs in the login.
Min. login length is 1.
Max. login length is 20.

Here is my Java code.

boolean checkLogin(String login) {
        final int maxLength = 20;
        if (login == null || login.isEmpty() || login.length() > maxLength) {
            return false;
        }

        final Pattern pattern = Pattern.compile("^[a-z][a-z\\d\\.\\-]*[a-z\\d]+$", Pattern.CASE_INSENSITIVE);
        final Matcher matcher = pattern.matcher(login);
        return matcher.matches();
}

Is everything OK?

That regex does not meet the requirements of your rules... so ... No, it is not OK.

The missing aspect is that the regex requires at least two characters, but the rules say one char is OK.

Also, I don't like that the last group has the + on it. While it is accurate in the sense that the last characters can be alpha/digit, it implies that the rule is for more than just the last character.

Another problem is that you allow logins of more than 20 characters.

A more meaningful/accurate regex would be:

^[a-z]([a-z\\d\\.\\-]{0,18}[a-z\\d])?$

Which requires 1 character, and then up to 18 'broad' characters followed by a constrained alpha/digit character.

EDIT:
2 things:

Aseem has rightly pointed out that the preceding if-condition essentially makes the {0,18} limit redundant. My preference is to keep it there for clarity, but, it does create duplication of logic which may lead to bugs later (How did I miss that the lenght is checked first... hmmm). Your call as to whether you should replace the {0,18} with just {*}.
The Pattern should be made static. There is no reason to re-compile the pattern for every call to the method (Patterns are thread-safe):

private static final Pattern pattern = Pattern.compile(
        "^[a-z]([a-z\\d\\.\\-]{0,18}[a-z\\d])?$", Pattern.CASE_INSENSITIVE);

private static final int maxLength = 20;

boolean checkLogin(String login) {

    if (login == null || login.isEmpty() || login.length() > maxLength) {
        return false;
    }

    return pattern.matcher(login).matches();
}


Check if a String is a valid Double For a class, I need to ask the user for a double. To verify that the number can be safely passed to Double.parseDouble, I wrote the following function. I'd like to have it heavily scrutinized; including the comments.

Are my comments helpful? They're more directed towards the marker than a maintainer down the road. Should I phrase them this way if it's for a class, and not "real code" that will need to be maintained later?
Is my assertion about the nature of charAt correct? Should I be caching str.charAt(i) in the loop?
I was rather proud of the first validity check, but found it to be kind of ugly when I looked at it the next day. Is there any better way that it could be written?
Anything else that could be improved. Be critical please!

As @h.j.k pointed out, this would be entirely easier by just catching the exception that Double.parseDouble throws, but there's no fun in that! I wanted the exercise.

    import static java.lang.Character.*;

    public static boolean strIsNumeric(String str) {
        //If the string is empty, or if it isn't a '-' or digit, return false.
        //We need a check like this or a prefixed '-' will prevent it from
        // being recognized as a negative number.
        //This is safe because logical-OR "short-circuits", the right hand will only
        // evaluate if the left is false. If this wasn't the case, the calls to "charAt(0)"
        // would cause an exception on empty String.
        if ( str.isEmpty() || (str.charAt(0) != '-' && !isDigit(str.charAt(0))) ) {
            return false;
        }

        //We're skipping the first char since it was checked above.
        boolean foundADecimal = false;
        for (int i = 1; i < str.length(); i++) {
            //To avoid repeated evaluations
            char c = str.charAt(i);

            //Check to see if there's at most 1 decimal
            if ( !foundADecimal && c == '.' ) {
                foundADecimal = true;

                //We've "verified" this char already as a valid decimal point,
                // so we need to skip the next "isDigit" check.
                continue;
            }

            if ( !isDigit(c) ) {
                return false;
            }
        }

        return true;
    }

There are three validation failures (ignoring case sensitivity) that Double.parseDouble() would have accepted otherwise:

A starting positive sign (+1.23).
A starting decimal point (.123).
The E notation aka scientific notation (1.23E4).

As for your comments... they should explain the why, not the how. The comments you have now are only explaining the how, so you should consider removing them when you get the chance to write code outside of academic exercises in the future...

Variable names... it depends. i is quite well-understood, but some may frown upon a single c to represent a character, or even str for your method argument... I'm not suggesting to be overly descriptive (currentCharacterOfMethodArgument is an example), but maybe an actual English word (current?) might be slightly more readable for your marker.

And just for completeness, remember to do a null check at the start too, before str.isEmpty(). :)

edit And since you asked about it, the implementation of charAt(int) is just:

public char charAt(int index) {
    if ((index < 0) || (index >= value.length)) {
        throw new StringIndexOutOfBoundsException(index);
    }
    return value[index];
}

The only 'evaluation' here is the if statement for out-of-bounds check, which I don't think is that significant/performance-hurting (if you're alluding to this indirectly as well). It's fine both ways whether you want to 'cache' it first.


Total number of common factors for two numbers LARGE VALUES up to 10^12 Inputs are two values \$1 \le m , n \le 10^{12}\$. I don't know why my code is taking so long for large values where the time limit is 1 sec. Please suggest some critical modifications.

#include <iostream>
#include<algorithm>
using namespace std;

int main() {
    long long count = 0, m, n;
    cin >> m >> n;

    for (long long int i = 1; i <= ((min(m, n)) / 2) + 1; i++) {
        if ((m%i == 0) && (n%i == 0)) {
            count++;
        }
    }

    if (((n%m == 0) || (m%n == 0)) && (n != m)) {
        count++;
    }

    cout<<count;

    return 0;
}

long long count = 0, m, n;
    ...
    for (long long int i = 1; i <= ((min(m, n)) / 2) + 1; i++) {

long long or long long int? I'm not a C++ programmer so I don't know whether style guides have a preference, but I'm quite certain that you should be consistent.

  Inputs are two values \$1 \le m , n \le 10^{12}\$. I don't know why my code is taking so long
  ...

   for (long long int i = 1; i <= ((min(m, n)) / 2) + 1; i++) {

There you go. The main loop runs up to \$5 \times 10^{11}\$ times. As a rule of thumb, when brute-forcing a problem I work on the basis that \$10^9\$ is about as high as I want to go.

Solution: change your algorithm. This will require learning some mathematics, and I'm not going to write a detailed tutorial, but there are two things you need:

Basic number theory: unique prime factorisation, greatest common divisor, finding it efficiently;
Sane factorisation of a small number (and \$10^{12}\$ is very small in these terms)

If you wanted to do it properly then you could extend the number theory research and learn about Euler's totient function.


Decorator Pattern in PHP Simple implementation I am learning decorator design pattern in PHP. Please see what i am doing is correct or wrong, would appreciate your review, feedback, comments, suggestions.

<?php
/**
 * NotifierService interface is used as abstraction for Notification Mechanism
 *
 * It can be later implemented as EmailNotifierService, PushNotifierService, or
 * any other Notification SErvice
 */
interface NotifierService {

    public function Notify();
}

/**
 *  EmailNotifierService - is Email Decorator
 *  here the EmailSending is Implemented, and it can be used wherever it is
 *  required, Different Unique services that doesn not have common behaviours
 *   are utilised using decorator patterns
 */
class EmailNotifierService implements NotifierService {

    public function __construct(){

        $this->to = 'Praveen@gmail.com' ;//$toarr;
        $this->from  = 'Napoleon@gmail.com' ;//$from;
        $this->msg   = 'This is a test message';//$msg
    }

    /**
     * [sendMail Actual Implementation of Sending Email is put here]
     * Kept simple for demo purposes
     */
    private function sendMail(){

        echo "Sending Email To" .PHP_EOL;
        print_r($this->to);

        echo "From: " .PHP_EOL;
        print_r($this->from);

        echo "Msg: " .PHP_EOL;
        print_r($this->msg);
    }

    /**
     * [Notify - Implementation of the interface method - Notify]
     */
    public function Notify(){
        echo "Notify function called" .PHP_EOL;
        $this->SendMail();
    }
}

class User implements NotifierService{

    var $users = [];
    var $notifierservice;

    //Passing NotifierService Object, So that we can utilise its service
    public function __construct(NotifierService $notifierservice){

        echo "New User constructed";
        $this->notifierservice = $notifierservice;
    }

    /**
     * [adduser This function creates a new user, like new user signup,
     * Whenever a new user signup, a Email is sent to the Website Admin]
     * @param  [Username] $u Username
     */
    public function adduser($u){
        $this->users[]=$u;
        $this->Notify();// Calls the Notification
    }

    public function Notify(){
        $this->notifierservice->Notify();
    }

}
// New User is created, EmailNotificationService is wrapped in it
// So that whenever a new user is created, Admin gets an Email
$u1 = new User(new EmailNotifierService());
$u1->adduser('Ramkumar');

Just because a class like User has a dependency on another class (like an objects instance of type NotifierService) it does not mean that class has to implement that same interface.  Here it seems that User should not implement this interface at all. You can remove the Notify() method on this class and just call the Notify() method on the passed dependency in your adduser() method.
I don't think decorator pattern makes sense at all for what you are trying to do here.  You are not really trying to "decorate" objects adhering to NotifierService interface with additional functionality related to that interface, but rather simply consume a class of this interface in User.  This is a straight dependency injection situation.
The class implementation of EmailNotifierService is pretty trivial here with hard-coded to/from/message values, almost to the point that this code does not meet code review criteria.  Surely this is not something that is production ready.
Don't echo out output from classes such as this. Leave that to some other functionality.  If you simply want to "log" out class activity, then actually log it (you know to application/server logs).
Why does a class called User deal with multiple users?  Should this class be renamed?
Don't start method/function names with uppercase letters like you do here with Notify() and SendMail().  This should be notify() and sendMail().
$notifierservice should probably be $notifierService (camel-cased). adduser() should be addUser().
You should consider adding validation on public methods, either through type hinting of parameters (like you do for the notifier dependency) or through guarding clauses at beginning of method.  Here, for example, you do nothing to ensure you are getting a valid user passed to your adduser() method.


Optimizing this maze-solver I've written C# code for solving a maze problem.  It works, but I want to optimize it so that it could give me all paths.  To solve the maze, I walk on the zeros and assumed that the final is reaching -3.

const int MAZE_SIZE = 6;
static int[] allowed_move_row = { 0, -1, 0, 1 };
static int[] allowed_move_col = { 1, 0, -1, 0 };
const int MAX_ALLOWED_MOVES = 4;
static int[,] Optmaze = new int[MAZE_SIZE, MAZE_SIZE];
static int[,] maze  = new int[MAZE_SIZE, MAZE_SIZE] {
                       { -2 , 0  ,  0   , 0  ,  0  ,  -1 },
                       { -1 , 0  , -1   , 0  , -1  ,  0 } ,
                       { -1 , -1 , -1   , 0  ,  0  ,  0 } ,
                       { -1 , 0  , -1   , -1 ,  0  ,  -1 },
                       { -1 , -1 , -1   , 0  ,  0  ,  0 } ,
                       { -1 , 0  , -1   , -3 ,  0  ,  0 } };

static bool Solve(int previous_row, int previous_col, int next_step_no)
{
     for (int i = 0; i < MAX_ALLOWED_MOVES; i++)
     {
          int col = previous_col + allowed_move_col[i];
          int row = previous_row + allowed_move_row[i];
          if (col < 0 || col >= MAZE_SIZE)
              continue;
          if (row < 0 || row >= MAZE_SIZE)
              continue;

          if (maze[row, col] == -3)
              return true;

          if (maze[row, col] != 0)
              continue;

          maze[row, col] = next_step_no;

          if (Solve(row, col, next_step_no + 1))
              return true;

          else maze[row, col] = 0;
     }
            return false;
}

static void Main(string[] args)
{
    Print(maze);
    Console.WriteLine("-----------Solve-------------");
    if (Solve (0, 0, 1) )
        Print( maze);
    else
        Console.WriteLine("No Result");
}

static void Print(int[,] maze)
{
    for (int i = 0; i < maze.GetLength(0); i++)
    {
        for (int j = 0; j < maze.GetLength(1); j++)
        {
            Console.Write(" " + String.Format("{0,3}",maze[i, j]) + " ");
        }
        Console.WriteLine();
    }
}

Outputting:

       -2    0    0    0    0   -1
       -1    0   -1    0   -1    0
       -1   -1   -1    0    0    0
       -1    0   -1   -1    0   -1
       -1   -1   -1    0    0    0
       -1    0   -1   -3    0    0
       -----------Solve-------------
       -2    1    2    3    0   -1
       -1    0   -1    4   -1    0
       -1   -1   -1    5    6    0
       -1    0   -1   -1    7   -1
       -1   -1   -1    0    8    9
       -1    0   -1   -3   11   10

Walking all zeros is indeed suboptimal. A good first optimization is to preferentially walk on the zeros that bring you closer to the end point.

To do this, at each step calculate the pythagorean distance between the x,y coords of the array entry you're considering walking to, and the x,y coords of the array entry containing -3. This will be the cost of walking to that square. Store all calculated costs so you don't have to recalculate them.

Then, continue to preferentially choose lowest-cost next squares until you reach the end or a dead end. In cases of dead ends, simply go back and choose the next lowest-cost square that has yet to be traversed. To do this, you will need to keep some notion of path hierarchy, i.e. which steps lead to which next potential steps. A custom node type will do this for you.

If you've made it this far, then congratulations! You have a rudimentary A* path-finding algorithm. For a helpful guide to implementing this algorithm I recommend this resource.

There are further enhancements to A* efficiency, such as the Jump Point Search algorithm, which is best for more open spaces.


Project Euler #5 in C++ I'm currently doing the Project Euler challenges to learn C++. The fifth problem is the following.

  2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.

  What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

I thought of a loop between 20 and 10 as everything under 10 is a multiple of something under 10. At each iteration, the result is the lcm of the loop variable and the current result.

If I am correct, the complexity of this should be \$\mathcal{O}(n/2)\$. Am I right?

Is there any faster/better/cleaner implementation or another algorithm with a better complexity?

This is my code.

#include <iostream>

long gcd(long a, long b)
{
    long t;

    if (b == 0) return a;

    while (b != 0) {
        t = b;
        b = a % b;
        a = t;
    }
    return a;
}

long lcm(long a, long b)
{
    return (a * b) / gcd(a, b);
}

int main()
{
    long n(20), result(1);
    for (long i = n; i > n/2; --i) {
        result = lcm(result, i);
    }
    std::cout << "Result: " << result << "\n";
}

Welcome to Code Review!  This is actually pretty good code for a beginner, but here are some things that may help you improve your program.

Simplify the code

The gcd routine looks like this:

long gcd(long a, long b)
{
    long t;

    if (b == 0) return a;

    while (b != 0) {
        t = b;
        b = a % b;
        a = t;
    }
    return a;
}

It can be simplified in three ways.  First, we can declare t within the loop.  Second, we can eliminate the if statement entirely.  Third, we can simplify the while loop condition to simply while (b) which is the same as while (b != 0):

long gcd(long a, long b)
{
    while (b) {
        long t = b;
        b = a % b;
        a = t;
    }
    return a;
}

Use const where appropriate

The value of n is constant, so it would make sense to declare it either const or even better constexpr.

Consider signed versus unsigned

It's always worth thinking about the domain of the numbers in a calculation.  In this case, it seems that all of the numbers are probably intended to be unsigned, but they're declared long which gives signed numbers.

Think of alternative algorithms and implementations

I think your algorithm is fast enough, but an alternative approach would instead be to calculate all of the unique prime factors of all of the numbers < 20 and simply multiply them together.  With the judicious use of constexpr, one could even calculate everything at compile-time which would make for a very fast calculation.  For inspiration, see Compile-time sieve of Eratosthenes


Array Search in Haskell I currently try to learn some Haskell to improve my overall programming skills. To practice, I implement the C course exercises that I very familiar with. One is this:

  Write a function that finds an array (needle) inside of another array (haystack). Return the index where the first match starts. If it cannot be found, return -1.

So in C, this is implemented using a couple of loops:

int array_find(char *hay, char *nee, int hay_len, int nee_len) {
    int hay_idx, nee_idx;
    for (hay_idx = 0; hay_idx < hay_len - nee_len; hay_idx++) {
        for (nee_idx = 0; nee_idx < nee_len; nee_idx++) {
            if (hay[hay_idx + nee_idx] != nee[nee_idx]) {
                break;
            }
        }
        if (nee_idx == nee_len) {
            return hay_idx;
        }
    }

    return -1;
}

The following is my attempt in Haskell. I made comments which probably state the obvious for Haskell experts but it should make my reasoning clear.

-- Public function, searches the haystack for a needle and returns the index
-- (zero-based).
arraySearch :: (Eq a) => [a] -> [a] -> Maybe Int
arraySearch h n = arraySearchNext (arraySearchBegin h n 0) h n 0

-- Searches the haystack starting from the next position if the last search has
-- failed.
arraySearchNext :: (Eq a) => Maybe Int -> [a] -> [a] -> Int -> Maybe Int
-- The previous search has found something, so that is the first occurence of
-- the needle.
arraySearchNext (Just i) _ _ _ = Just i
-- The previous search has not given us anything but we are out of haystack.
-- Therefore nothing could be found.
arraySearchNext Nothing [] _ _ = Nothing
-- The previous search has not found anything but there is some haystack left.
-- We will try the search again by shifting by one and then shift again.
arraySearchNext Nothing (h:hs) n i =
    arraySearchNext (arraySearchBegin hs n (i + 1)) hs n (i + 1)

-- Looks whether the needle can be found at the beginning of the haystack.
arraySearchBegin :: (Eq a) => [a] -> [a] -> Int -> Maybe Int
-- In case we managed to finish the needle, we must have found something.
arraySearchBegin _ [] i = Just i
-- In case the haystack is empty, but the needle is not (pattern above), we
-- have not found anything.
arraySearchBegin [] _ _ = Nothing
-- There is still some haystack and needle left. If the first element each
-- matches, we must go on. Otherwise this won't be a match.
arraySearchBegin (h:hs) (n:ns) i
    | h == n        = arraySearchBegin hs ns i
    | otherwise     = Nothing

main = print $ arraySearch [1..10] [5..6]

Since there are so many lines and I have a bunch of helper functions, I wonder whether there is some better way‽

You use explicit recursion, which is fun, but not always necessary. arraySearchBegin can be expressed with zipWith and and, for example:

arraySearchBegin :: (Eq a) => [a] -> [a] -> b -> Maybe b
arraySearchBegin hs ns k
  | and (zipWith (==) hs ns) = Just k
  | otherwise                = Nothing

But that's still to explicit. Data.List contains isPrefixOf, which is exactly what we need:

import Data.List (isPrefixOf)

arraySearchBegin :: (Eq a) => [a] -> [a] -> b -> Maybe b
arraySearchBegin hs ns k
  | ns `isPrefixOf` hs = Just k
  | otherwise          = Nothing

That being said, arraySearchBegin's name is misleading. [a] is a linked list, not an array. We should probably rename it, but we won't use it in the final variant of arraySearch.

Next, we don't need the Maybe Int in arraySearchNext:

arraySearchNext :: (Eq a) => [a] -> [a] -> Int -> Maybe Int
arraySearchNext []         ns _ = Nothing
arraySearchNext hss@(_:hs) ns i
  | ns `isPrefixOf` hss = Just i
  | otherwise           = arraySearchNext hs ns (i + 1)

We can stop as soon as our needle is a prefix of our haystack. However, we're basically checking all tails of hss here. And there is again a function in Data.List that yields those tails for us:

ghci> import Data.List
ghci> tails [1..5]
[[1,2,3,4,5],[2,3,4,5],[3,4,5],[4,5],[5],[]]

We therefore just check which one is the first list in tails haystack that can be prefixed by needle. If we zip the indexes with the tails we're able to write arraySearch  as a list comprehension:

import Data.Maybe (listToMaybe)
import Data.List (isPrefixOf, tails)

search :: Eq a => [a] -> [a] -> Maybe Int
search hs ns = listToMaybe [ idx | (idx, hss) <- zip [0..] tails hs
                                 , ns `isPrefixOf` hss
                           ]

But Data.List contains findIndex :: (a -> Bool) -> [a] -> Maybe Int, which brings us to:

import Data.List (isPrefixOf, tails, findIndex)

search :: Eq a => [a] -> [a] -> Maybe Int
search hs ns = findIndex (ns `isPrefixOf`) (tails hs)

This however needs knowledge of the standard library. So how do you find those functions if you don't know them yet? Welcome to Hoogle. We can find findIndex by searching for (a -> Bool) -> [a] -> Maybe Int, isPrefixOf by Eq a => [a] -> [a] -> Bool and tails by [a] -> [[a]].

If we didn't have any of those helpers, your code would be fine, except for the strange explicit result in arraySearchNext's first argument. That was too complex, as you will stop as soon as you find a Just either way.


Find "secure file erase" commands on the system This code finds which "secure file erase" command exists on the system.  It used to simply test for "shred" and pick "rm" as a default.  Now it is a bit more complex since I've ported the code to operating systems with "srm" and "rm -P".  Is this still cleanly written?  How can I improve it?
var shredPath, shredFlag string // Memoization cache

// shredCmd determines which command to use to securely erase a file. It returns
// the command to run and what flags to use with it. Determining the answer
// can be slow, therefore the answer is memoized and returned to future callers.
func shredCmd() (string, string) {
    // Use the memoized result.
    if shredPath != "" {
        return shredPath, shredFlag
    }

    var path string
    var err error
    if path, err = exec.LookPath("shred"); err == nil {
        shredPath, shredFlag = path, "-u"
    } else if path, err = exec.LookPath("srm"); err == nil {
        shredPath, shredFlag = path, "-f"
    } else if path, err = exec.LookPath("rm"); err == nil {
        shredPath, shredFlag = path, "-f"
        // Does this command support the "-P" flag?
        tmpfile, err := ioutil.TempFile("", "rmtest")
        defer os.Remove(tmpfile.Name()) // clean up
        err = RunBash("rm", "-P", tmpfile.Name())
        if err != nil {
            shredFlag = "-Pf"
        }
    }

    // Single exit, so we don't have to repeat the memoization code.
    return shredPath, shredFlag
}

// RunBash runs a Bash command.
func RunBash(command string, args ...string) error {
    cmd := exec.Command(command, args...)
    cmd.Stdin = os.Stdin
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    err := cmd.Start()
    if err != nil {
        log.Fatal(err)
    }
    err = cmd.Wait()
    if err != nil {
        return fmt.Errorf("RunBash cmd=%q err=%w", command, err)
    }
    return nil
}
```

You have no synchronization for these package variables
var shredPath, shredFlag string

Therefore,
fmt.Println(shredPath, shredFlag)
go func() {
    for i := 0; i < 1000; i++ {
        shredCmd()
    }
}()
for i := 0; i < 1000; i++ {
    shredCmd()
}
fmt.Println(shredPath, shredFlag)

WARNING: DATA RACE
Found 3 data race(s)

os.Remove() and commands may fail if the file is open.
If you encounter something unexpected, for example Windows, you do nothing, not even os.Remove().
The if statements are ugly. Since shredCmd() is effectively a test, use the standard testing package technique, table-driven tests.
You provide no context for usage or testing, for example
func ShredFile(filename string) error {
    // ...
}

And so on.

Here's an attempt to fix some of the issues:
shred.go:
package main

import (
    "errors"
    "fmt"
    "io/ioutil"
    "os"
    "os/exec"
    "time"
)

var shredCmds = []struct {
    name, opts string
}{
    {"shred", "-u"},
    {"srm", "-f"},
    {"rm", "-Pf"},
    {"rm", "-f"},
    {"sdelete", "-a"},
}

func shredTemp(path, opts string) error {
    file, err := ioutil.TempFile("", "shredTemp.")
    if err != nil {
        return err
    }
    filename := file.Name()
    defer os.Remove(filename)
    defer file.Close()

    err = file.Close()
    if err != nil {
        return err
    }
    err = RunCmd(path, opts, filename)
    if err != nil {
        return err
    }
    return nil
}

var shredPath, shredOpts = func() (string, string) {
    for _, cmd := range shredCmds {
        path, err := exec.LookPath(cmd.name)
        if err != nil {
            continue
        }
        err = shredTemp(path, cmd.opts)
        if err == nil {
            return path, cmd.opts
        }
    }
    return "", ""
}()

func ShredFile(filename string) error {
    fi, err := os.Stat(filename)
    if err != nil {
        return err
    }
    if !fi.Mode().IsRegular() {
        err := errors.New("filename is not mode regular")
        return err
    }

    if shredPath == "" {
        return os.Remove(filename)
    }

    err = RunCmd(shredPath, shredOpts, filename)
    if err != nil {
        return err
    }
    return nil
}

func RunCmd(command string, args ...string) error {
    cmd := exec.Command(command, args...)
    cmd.Stdin = os.Stdin
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    err := cmd.Start()
    if err != nil {
        return fmt.Errorf("RunCmd cmd=%q err=%w", command, err)
    }
    err = cmd.Wait()
    if err != nil {
        return fmt.Errorf("RunCmd cmd=%q err=%w", command, err)
    }
    return nil
}

func main() {
    file, err := ioutil.TempFile("", "ShredFile.")
    if err != nil {
        fmt.Println(err)
        return
    }
    filename := file.Name()
    file.Close()
    defer os.Remove(filename)
    start := time.Now()
    err = ShredFile(filename)
    since := time.Since(start)
    fmt.Println(filename, since, err)
}


Breadth-first tree cutting I have a working solution for 'Cut the tree' problem but it is not fast enough.

  Problem Statement:

  Atul is into graph theory, and he is learning about trees nowadays. He observed that the removal of an edge from a given tree T will result in the formation of two separate trees, T1 and T2.

  Each vertex of the tree T is assigned a positive integer. Your task is to remove an edge, such that the Tree_diff of the resultant trees is minimized. Tree_diff is defined as the following:

 F(T) = Sum of numbers written on each vertex of a tree T
 Tree_diff(T) = abs(F(T1) - F(T2))

  Input Format:

  The first line will contain an integer \$N\$, i.e. the number of vertices in the tree.

  The next line will contain \$N\$ integers separated by a single space, i.e. the values assigned to each of the vertices.

  The next \$N−1\$ lines contain a pair of integers each, separated by a single space, that denote the edges of the tree.

  In the above input, the vertices are numbered from \$1\$ to \$N\$.

  Output Format:

  A single line containing the minimum value of Tree_diff.

  Constraints:

  \$3≤N≤105\$
  \$1≤\$ number written on each vertex \$≤1001\$

  Sample Input:

6
100 200 100 500 100 600
1 2
2 3
2 5
4 5
5 6

  Sample Output:

400

  Explanation:

  Originally, we can represent tree as

         1(100)
          \
           2(200)
          / \
    (100)5   3(100)
        / \
  (500)4   6(600)

Cutting the edge at 1 2 would result in Tree_diff = 1500-100 = 1400
Cutting the edge at 2 3 would result in Tree_diff = 1500-100 = 1400
Cutting the edge at 2 5 would result in Tree_diff = 1200-400 = 800
Cutting the edge at 4 5 would result in Tree_diff = 1100-500 = 600
Cutting the edge at 5 6 would result in Tree_diff = 1000-600 = 400

  Hence, the answer is 400.

The solution is in Python, with iterative BFS. I could only pass the first five tests and timed out on the rest. Any hints for improving time complexity? Input size can be up to \$100,000\$ vertices.

def find_sum(v, p):
    s = vertices[v-1]
    stack  = [(v,p)]
    while stack:
        v,p = stack.pop()
        for e in adj_list[v]:
            if p != e:
                s += vertices[e-1]
                stack.append((e,v))
    return s

n = int(raw_input())
vertices = map(int, raw_input().split())
adj_list = {}
edges = []
total = sum(vertices)
res = total
for _ in xrange(n-1):
    u, v = map(int, raw_input().split())
    edges.append((u,v))
    if u not in adj_list:
        adj_list[u] = []
    if v not in adj_list:
        adj_list[v] = []
    adj_list[v].append(u)
    adj_list[u].append(v)

for u,v in edges:
    tmp = find_sum(u,v)
    res = min(res, abs(tmp-abs(total-tmp)))
print res

For every edge in your graph, you are doing a BFS of the tree on one side of the edge to sum all of its vertex weights. It is not hard to see that you are doing a lot of work over an over again. Imagine that every vertex in your tree was connected to only 2 other vertices, so that it basically looked like a doubly linked list. If we store the list in an array, your code would be doing something akin to:

def min_diff_cut(list_):
    total = sum(list_)
    best_diff = total
    for idx in range(1, len(list_)):
        best_diff = min(best_diff, total - sum(list[:idx]))
    return best_diff

This of course has \$O(n^2)\$ complexity, which is rarely a good thing. A much more efficient implementation running in \$O(n)\$ time would do something like:

def min_diff_cut(list_):
    total = sum(list_)
    best_diff = total
    subtotal = 0
    for value in list_:
        subtotal += value
        best_diff = min(best_diff, total - subtotal)
    return best_diff

How can you expand this idea to work on your possibly branched tree? Well, if instead of breadth first, you go depth first, you can recursively find out what the best diff and sum of weights down an edge is, and come up with a solution from there. This would look something like:

def min_diff_cut(graph, weights):
    total = sum(weights)
    visited = set()

    def subtree_total_weight(vertex):
        visited.add(vertex)
        subdiff = total
        subtotal = weights[vertex]
        for next_vtx in graph[vertex]:
            if next_vtx in visited:
                continue
            subt, subd = subtree_total_weight(next_vtx)
            subtotal += subt
            subdiff = min(subdiff, subd)
        return subtotal, min(subdiff, abs(2*subtotal - total))

    # We are assuming vertices are 0-based indices
    _, best_diff = subtree_total_weight(0)

    return best_diff

if __name__ == '__main__':
    print min_diff_cut({0: [1], 1: [0, 2, 4], 2: [1], 3: [4],
                        4: [1, 3, 5], 5: [4]},
                       [100, 200, 100, 500, 100, 600])


Fake bank account class I just some reviews on how I can make it more efficient or cleaner and what I should fix in general.

class bankAcc
{
public:
    bankAcc() = default;
    bankAcc(string fn, string ln) : firstName(fn), lastName(ln) {}
    bool createUser(istream &is);
    bool login() const;
private:
    bool correctLogin(const string&, const string&) const;
    static vector<string> usernames;
    static vector<string> passwords;
    string firstName, lastName;
    string username, password;
};

vector<string> bankAcc::usernames;
vector<string> bankAcc::passwords;

bool bankAcc::createUser(istream &is) {
    is >> username >> password;
    if (is.fail()) {
        cout << "Invalid option" << endl;
        cin.clear();
        return false;
    }
    usernames.push_back(username);
    passwords.push_back(password);
    return true;
}

bool bankAcc::login() const {
    bool login;
    string tempUser, tempPass;
    cin >> tempUser >> tempPass;
    if (cin.fail()) {
        cout << "Invalid options" << endl;
        cin.clear();
        return false;
    }
    login = correctLogin(tempUser, tempPass);
    if (login == true) {
        cout << "Logged on" << endl;
        return login;
    }
    else {
        cout << "Incorrect info" << endl;
        return login;
    }
    return login;
    }

bool bankAcc::correctLogin(const string &tempUser, const string &tempPass) const {

    if (usernames.empty() || passwords.empty())
        return false;
    bool usernamePassed, passwordPassed; int cnt = 0;
    for (auto c : usernames) {
        if (tempUser == c) {
            usernamePassed = true;
            break;
        }
        else {
            usernamePassed = false;
        }
        ++cnt;
    }
    if (passwords[cnt] == tempPass && cnt < passwords.size()) {
        passwordPassed = true;
    }
    else {
        passwordPassed = false;
    }
    return usernamePassed && passwordPassed;
}

I have a few suggestions:

In your constructor, you are passing in string by copy when it is completely unnecessary to do so. You should pass these in by const reference:

bankAcc(const string &fn, const string &ln) { /*...*/ }

Also, this constructor should check whether or not fn or ln are clean inputs (i.e. they're valid first and last names). You can do this like so:

bankAcc(const string &fn, const string &ln)
{
    if (fn == "" || ln == "")
        throw std::invalid_argument("Can't have empty names");
    firstName = fn;
    lastName = ln;
}

Speaking of firstName and lastName, I don't see a use for them here at all as they're only used in the constructor and nowhere else. Is there a need for them?

You should also practice the Principle of Sole Responsibility here. In other words, each class should have only one single responsibility to take care of. Your class is called bankAcc yet it really seems more like a BankAccountLoginValidator class to me. Your class isn't doing anything that a bank account should be doing (no balance, no withdrawal/deposit schemes etc). Moreover, you should generally separate I/O operations from the validation code in this class. Have the caller of the class objects perform the I/O and then have them use your class to validate their input. Your class should not be responsible for I/O.

Finally, your use of two vectors to store the user-name and password is inefficient since you end up spending O(n) time searching for the right username in the usernames vector. Instead, look at using a hashtable to store this information instead. A hash-table in C++11 is called std::unordered_map. Then, your lookup operation can be done in O(1) amortized time. With this map, your correct_login() function becomes unnecessary.

Here's your code refactored a bit:

class bankAcc
{
public:
    bankAcc() = default;
    bankAcc(const string &fn, const string &ln)
    {
        if (fn == "" || ln == "")
            throw std::invalid_argument("Error! Can't have empty names");
        firstName = fn;
        lastName = ln;
    }
    bool createUser(const string &user_name, const string &pass_word)
    {
        if (user_name == "" || pass_word == "")
            return false;
        /* Maybe add more validation code here */
        // ....

        uname_to_pass.insert(user_name, pass_word);
    }
    bool login(const string & user_name, const string &pass) const
    {
        return uname_to_pass[user_name] == pass;
    }

private:
    static unordered_map<string, string> uname_to_pass;
    string firstName, lastName;
    string username, password;
};


Function that returns the number of items that appear more than once This is the question:

  Write a method that takes in a string and returns the number of letters that appear more than once in the string. You may assume the string contains only lowercase letters. Count the number of  letters that repeat, not the number of times they repeat in the string.

How can I make this better?

function count(str){
    var nArr = [];
    var result = [];
    var obj = {};
    var arr = str.split("");
    // count all occurances in arr
    for(var i = 0; i < arr.length; i++){
        var letter = arr[i];
        obj[letter] === undefined ? obj[letter] = 1 : obj[letter] += 1;

    }
    for(var prop in obj){
        if(obj[prop] !== 1){
            nArr.push(prop);
        }
    }
    return nArr.length;

}

count("adsaaaffda");

The name count is too vague for such a specific task.  I suggest countReappearingChars instead.

Avoid big blocks of variable definitions.  Code is easier to read and maintain when variables are declared and initialized near where they are used.  For example, with a block like this, it's harder to notice that result is never used.

nArr does not need to be an array.  You can just keep a count.

obj is cryptically named.  I suggest calling it charCount.

var arr = str.split("") is unnecessary; you can iterate over str directly.

"occurances" is misspelled in the comment.

This line is hard to read:

obj[letter] === undefined ? obj[letter] = 1 : obj[letter] += 1;

It would be more clearly be written using if-else, and therefore should be:

if (obj[letter] === undefined) {
    obj[letter] = 1;
} else {
    obj[letter] += 1;
}

I would consider your ternary operator to be abusive, since you are not using it as as an expression.  In contrast, this expression could be considered appropriate, since the right-hand side of the assignment is an expression:

obj[letter] = (obj[letter] === undefined) ? 1 : obj[letter] + 1;

But that still looks cumbersome.  If you want to get clever with it, you could use || 0, as shown below.

function countReappearingChars(str) {
    // Count all occurrences in str
    var charCount = {};
    for (var i = 0; i < str.length; i++) {
        charCount[str[i]] = 1 + (charCount[str[i]] || 0);
    }

    // Loop over and filter charCount
    var result = 0;
    for (var char in charCount) {
        if (charCount[char] > 1) result++;
    }
    return result;
}


recursive file search, C , linux hi guys this script recursively searches for files with a certain extension and possibly in a directory passed by the terminal ....(In case no directory has been passed, search from the current one) I believe it works correctly but I wanted to ask you if you noticed any flaws and report them to me or just tell me that everything is ok ...thanksss

 #include<stdio.h>
 #include<sys/stat.h>
 #include<errno.h>
 #include<stdlib.h>
 #include<dirent.h>
 #include<stdarg.h>
 #include<limits.h>
 #include<string.h>
 #include<time.h>

 char * scrivi(const char * a, char * B) {

 char *targetdir = malloc(2048);
 strcpy(targetdir,a);
 strcat(targetdir,"/");
 strcat(targetdir,B);
 return targetdir;
 }

 void ricor1(const char estensione[],const char nomedirectory[]){

 struct stat attr;
 char dbuf[PATH_MAX+1];
 DIR * fh ;
 struct dirent *fdata;
 struct stat buf;

 if((fh=opendir(nomedirectory))==NULL){
 perror("ERRORE 1");
 exit(errno);
 }

 while((fdata = readdir (fh))!=NULL){
 if(strcmp(fdata->d_name,".")==0){
 continue;
 }

 if(strcmp(fdata->d_name,"..")==0){
 continue;
  }

 char *percorso;
 percorso = scrivi(nomedirectory,fdata->d_name);
 if (fdata->d_type==DT_DIR){

 ricor1(estensione,percorso);
  }
 if (fdata->d_type == DT_REG && strstr(fdata->d_name, estensione)) {

realpath(percorso, dbuf);
printf("[%s]", dbuf);

stat(percorso, &attr);

printf("%s\n", ctime(&attr.st_mtime));

 }

free(percorso);
 }
closedir(fh);
 }

int main(int argc, char *argv[]) {

if(argc==3){

printf("Checking existence directory.. \n");

DIR* dir=opendir(argv[2]);

if(dir){
closedir(dir);
ricor1(argv[1],argv[2]);

 }

else if (ENOENT==errno){

printf("Directory doesn't exist\n");
  }

else{

printf("error");

     }
 }

else if(argc==2){

ricor1(argv[1],"./");

  }
 }

... if you noticed any flaws ....

Problems with scrivi()

char * scrivi(const char * a, char * B) {

 char *targetdir = malloc(2048);
 strcpy(targetdir,a);
 strcat(targetdir,"/");
 strcat(targetdir,B);
 return targetdir;
 }

Magic number

Why 2048?  Big enough - who knows?  Makes more sense to find the size needed and allocate rather than risk undefined behavior of a buffer overflow with strcpy(targetdir,a);

strcpy(), strcat(), strcat()

Walking down the string 3 times.  Bad enough to do twice.   (With a larger code re-write, we could avoid all repetitive walks down a, that is far below.)

char * B better as const char * b

Use const.

No error checking

With a recursion function, out-of-memory is a concern.

Alternative

char * scrivi(const char *a, const char *b) {
  size_t a_len = strlen(a);
  size_t b_len = strlen(b);
  char *targetdir = malloc(a_len + 1 + b_len + 1);

  if (targetdir) {
    memcpy(targetdir, a, a_len);
    targetdir[a_len] = '/';
    memcpy(targetdir + a_len + 1, b, b_len + 1);
  }
  return targetdir;
}

Re-write trick

Rather than repetitive malloc()/free(), allocate once ever for a working file path buffer.

size_t file_path_size = PATH_MAX * 2;  // Let us be generous.
char file_path = mallloc(file_path_size);

Before code calls ricor1() the first time, fill in the directory path and pass how much used, how much total.

strcpy(file_path, argv[2]);
ricor1(argv[1], file_path, strlen(argv[2]), file_path_size);

When forming the the full path, rather than percorso = scrivi(nomedirectory, fdata->d_name); with its allocations, write at the right place.

int length = snprintf(file_path + used, file_path_size - used, "/%s", fdata->d_name);

When recursing, append the sub-directory name and re-curse with an updated length

ricor1(estensione, file_path, used + length, file_path_size);

Include ample size error checking.

Consider code only ever needs   1 file_path[] at a time.


htaccess with compression, caching, and rewrite rules for SEO I have the following htaccess and I want it to be the perfect htaccess for my website so that it uses less bandwidth and meets the requirements of a good SEO.

I am on an apache server and my files are html, php, js(minified and normal), css, ttf fonts, images.

How can I improve this htaccess file?

RewriteBase /
RewriteEngine On

RewriteCond %{REQUEST_FILENAME}.php !-f
RewriteRule ^room/([A-Za-z0-9]+)$ playground.php?room=$1 [L]

RewriteCond %{REQUEST_FILENAME}.php !-f
RewriteRule ^join/([A-Za-z0-9]+)$ index.php?room=$1&action=join [L]

RewriteCond %{REQUEST_FILENAME}.php !-f
RewriteRule ^([\w-]+)$ $1.php [L]

<IfModule mod_deflate.c>
  # Compress HTML, CSS, JavaScript, Text, XML and fonts
  AddOutputFilterByType DEFLATE application/javascript
  AddOutputFilterByType DEFLATE application/vnd.ms-fontobject
  AddOutputFilterByType DEFLATE application/x-font
  AddOutputFilterByType DEFLATE application/x-font-opentype
  AddOutputFilterByType DEFLATE application/x-font-otf
  AddOutputFilterByType DEFLATE application/x-font-truetype
  AddOutputFilterByType DEFLATE application/x-font-ttf
  AddOutputFilterByType DEFLATE application/x-javascript
  AddOutputFilterByType DEFLATE application/xhtml+xml
  AddOutputFilterByType DEFLATE application/xml
  AddOutputFilterByType DEFLATE font/opentype
  AddOutputFilterByType DEFLATE font/otf
  AddOutputFilterByType DEFLATE font/ttf
  AddOutputFilterByType DEFLATE image/svg+xml
  AddOutputFilterByType DEFLATE image/x-icon
  AddOutputFilterByType DEFLATE text/css
  AddOutputFilterByType DEFLATE text/html
  AddOutputFilterByType DEFLATE text/javascript
  AddOutputFilterByType DEFLATE text/plain
  AddOutputFilterByType DEFLATE text/xml

  # Remove browser bugs (only needed for really old browsers)
  BrowserMatch ^Mozilla/4 gzip-only-text/html
  BrowserMatch ^Mozilla/4\.0[678] no-gzip
  BrowserMatch \bMSIE !no-gzip !gzip-only-text/html
  Header append Vary User-Agent
</IfModule>

#Cache-control
<IfModule mod_headers.c>
    <FilesMatch ".(jpg|jpeg|png|gif|ico)$">
       Header set Cache-Control "max-age=2628000, public"
    </FilesMatch>
    <FilesMatch ".(css|js)$">
       Header set Cache-Control "max-age=604800, public"
    </FilesMatch>
</IfModule>

The first question I always ask when considering using an htaccess file is - does this really need to be in an htaccess file?  By that I mean ask yourself a few questions:

Do you have the ability to configure Apache on your host? (i.e. you are not in some shared hosting where htaccess is your only recourse.)
Is this configuration common to all directories in your application?
Can you restart Apache upon any necessary configuration changes?

If you answered "yes" to all of these questions, you should have this configuration in your main Apache server config, rather than in a directory context, and simply turn off directory overrides. This eliminates the performance hit of checking every directory for an htaccess file on every request that hits this server.  This is the best way of "improving" most htaccess files. Site performance can impact your SEO.

As long as you answered "yes" to the first question, you should still probably consider moving this configuration into Apache config as long as you don't have unusual needs such as not being able to restart server without complications (either on your site or possibly other sites on same servers) or having a high  frequency of change to this configuration logic such that you would be restarting the server too frequently.

You can also enter directory-specific configuration in the main Apache config, so typically just because you have different configs for different directories, this alone should not be a reason to stick with htaccess.

If you are using eTags and have multiple host serving traffic, turn of the iNodes component in generating the eTags so you get more consistent eTag values across servers, so long as your are appropriately deploying or "touching" your files to match modification times across servers.

i.e.

FileETag MTime Size

Also, if you are not using eTags, you might consider it such that, even if a cached item has passed expiry, the browser still may not need to download the item if the eTags haven't changed.

If you are worried about server bandwidth and overall performance consider:

moving your static assets onto a CDN which is better suited for delivery of the content to end user browsers;
minifying all applicable assets (i.e. don't have some JS that is not minified). Minification is different that deflation of course and more effective regarding overall download size.
using much longer expiry times for your static assets (i.e. many years) along with an asset versioning strategy to deal with browser caching behavior issues. Think in terms of when you are deploying a static asset, you are deploying it forever (file storage is cheap).
cusing SVG as part of your suite of tools for replacing iconography, sprite sheets, and other vector-friendly images. I know this is getting more into application design. :)


Iterate over items in a set of dicts I'm using the following code to initialise 8 I/O-ports (i.e. GPIO pins on a Raspberry Pi). That's 4 x 2 pins, each duo controls a switch, A to D, and each duo consists of a pin for On and a pin for Off.

def init_switches():
    switch = {
        'A': dict(On = 22, Off = 12),
        'B': dict(On = 19, Off = 11),
        'C': dict(On = 18, Off = 16),
        'D': dict(On = 15, Off = 13)
        }

    # define all individual pins as output
    for ID in switch:
        for pin in switch[ID]:
            GPIO.setup(switch[ID][pin], GPIO.OUT)

    return switch

I chose this way, a set of dicts, because it allows me to say:

switch = init_switches()
switch['A']['On']

The last line makes for nice readable code. However the nested for loop is not so elegant. Isn't there a more concise way?

Edit: I'm deliberately iterating over keys to get to values. Values are pin numbers. Pin numbering is not arbitrary, but has to do with hardware design. It came out this way because it allowed the easiest circuitry to the switch controller. I'm deliberately hiding all that from the coding, because it's a different chapter altogether.

In both nested loops, it seems you really need the values of dictionaries.
You iterate over the keys just to use them to get the values:

for ID in switch:
    for pin in switch[ID]:
        GPIO.setup(switch[ID][pin], GPIO.OUT)

A more direct approach would be to iterate over the values instead of the keys:

for pins in switch.values():
    for pin in pins.values():
        GPIO.setup(pin, GPIO.OUT)

A minor thing, the definition of switch doesn't follow PEP8,
because of the spaces around the = when setting the dictionary keywords.
This is the recommended style:

switch = {
    'A': dict(On=22, Off=12),
    'B': dict(On=19, Off=11),
    'C': dict(On=18, Off=16),
    'D': dict(On=15, Off=13)
}


Webscraping from yellowbook I'm fairly good at programming python but of course i want to write cleaner code.

This is a simple script that scrapes leads off yellowbook.

I want to utilize list comprehensions but im comfortable with using for loops, however i can see how it can be repetitive.

Any ways i can make this cleaner ?

service_name = input("Input Industry: ")
city = input("Input The City: ")

class Item(scrapy.Item):
    business_name = scrapy.Field()
    phone_number = scrapy.Field()
    website = scrapy.Field()

class QuotesSpider(scrapy.Spider):
    name = "quotes"

    start_urls = [
        "http://www.yellowbook.com/s/" + service_name + "/" + city
    ]
    def __init__(self):
        self.seen_business_names = []
        self.seen_websites = []

    def parse(self, response):
        for business in response.css('div.listing-info'):
            item = Item()
            item['business_name'] = business.css('div.info.l h2 a::text').extract()
            item['website'] = business.css('a.s_website::attr(href)').extract()
            for x in item['business_name'] and item['website']:
                #new code here, call to self.seen_business_names
                if (x not in self.seen_business_names and x not in self.seen_websites):
                    if item['business_name']:
                        if item['website']:
                            item['phone_number'] = business.css('div.phone-number::text').extract_first()
                            yield item
                            self.seen_business_names.append(x)

        # next_page = response.css('div.pagination a::attr(href)').extract()
        for href in response.css('ul.page-nav.r li a::attr(href)'):
            yield response.follow(href, self.parse)

First: a general observation - simple nested if statements are equivalent to a single if with statements joined by and.

So:

if a:
    if b:
        if c:

Is equivalent to:

if a and b and c:

Second: You have an if test to see if business_name and website exist - but you do a lot of other things before this test. You can move this higher up so you 'escape' that code faster if you do something like:

item['business_name'] = business.css('div.info.l h2 a::text').extract()
item['website'] = business.css('a.s_website::attr(href)').extract()
if item['business_name'] and item['website']:
    for x in item['business_name'] and item['website']:
        #new code here, call to self.seen_business_names
        if (x not in self.seen_business_names and x not in self.seen_websites):
            item['phone_number'] = business.css('div.phone-number::text').extract_first()
            self.seen_business_names.append(x)
            yield item

(I've put the append before the yield as this seems slightly clearer to me).

Third - I don't think you mean for x in item['business_name'] and item['website']:

Rather can do either of:

for x in item['business_name'] + item['website']:

Or:

from itertools import chain
for x in chain(item['business_name'], item['website']):


Taking arbitrary length input in C Instead of having to use something like char buf[100500] and hope that no possible user input could be longer than 100500 bytes, I decided to make the following function:

char* input (FILE* in, size_t size)
{
    char * input_str = NULL;
    int c;
    size_t len = 0;

    /*initial allocation*/
    input_str = malloc(size);
    if (!input_str) return NULL;

    while ((c = fgetc(in)) != EOF && c != '\n')
    {
        input_str[len++] = c;

        /*allocate more room if needed*/
        if (len == size)
        {
            input_str = realloc(input_str, size += 16);
            if (!input_str) return NULL;
        }
    }

    input_str[len++] = 0;

    return realloc(input_str, len);
}

My questions about this code:

Is leaving it up to the calling function to free memory a bad idea?
Is there a way to increase performance for this (memory pool, etc)? I don't like making so many syscalls...
In general, is there a better way to go about this?

A simple improvement to improve performance by reducing the number of reallocs is to start with a decently sized buffer and when it fills up, grow it by a multiple of the size. For example, you could double the size when it fills up.

Of course this approach is less efficient memory wise, but in order to mitigate that, we can multiply by a smaller multiple (like 1.5x) and/or start with a smaller base size. If you don't care much about memory, you can up the multiple and up the base amount.

For truly enormous files, you will want to chunk your data and process it by chunk instead trying to load it into memory all at once. Another possibility is to just process it as it comes in via an on-line algorithm.

On a side note, it seems like you are reimplementing getline()'s functionality, so unless you need some special behavior, I'd recommend using that instead.


Efficient way to calculate the sum of all primes between a range Problem
I'm trying to write a code in which Q questions are asked and each question contains two numbers A and B. For each question, I have to find the sum of all primes between integers A and B (inclusive). I've written a program that works, but it isn't fast enough to pass all the restrictions. Is there any way to improve the efficiency of my code?
Restrictions
1≤Q≤10^5
1≤A≤B≤10^5
Time limit: 0.6s
import java.io.*;
import java.util.*;

public class sumofprimes{
    static BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    static PrintWriter pr = new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.out)));
    static StringTokenizer st;
    static boolean isPrime(int n){
        if(n==1)return false;
        for (int i=2; i*i<=n; i++)
            if (n%i==0) return false;
        return true;
    }

    static int sum(int a, int b){
        int sum=0;
        for(int i=b; i>=a; i--) {
            boolean prime=isPrime(i);
            if(prime) sum+=i;
        }
        return sum;
    }
    public static void main(String[] args)throws IOException{
        int q=readInt();
        for(int i=0; i<q; i++){
            int a=readInt(), b=readInt();
            System.out.println(sum(a,b));
        }
    }
    static String next () throws IOException {
        while (st == null || !st.hasMoreTokens())
           st = new StringTokenizer(br.readLine().trim());
    return st.nextToken();
    }
    static int readInt () throws IOException {
    return Integer.parseInt(next());
    }
}

Nice implementation, it's already efficient but as noted in the comments can be improved. Few general suggestions:

Format the code: many IDE support auto formatting or you can use an online formatter.

Omitting brackets is not good practice: it's better to include the brackets to avoid confusion when reading or changing the code. See the difference from:
static boolean isPrime(int n){
      if(n==1)return false;
      for (int i=2; i*i<=n; i++)
          if (n%i==0) return false;
      return true;
}

To:
static boolean isPrime(int n) {
    if (n == 1) {
        return false;
    }
    for (int i = 2; i * i <= n; i++) {
        if (n % i == 0) {
            return false;
        }
    }
    return true;
}

Arrange the methods from general to specific, it's easier to follow. From:
static boolean isPrime(int n){
}

static int sum(int a, int b){
    //call isPrime
}

public static void main(String[] args)throws IOException{
    // call readint
    // call sum
}

static String next () throws IOException {
}

static int readInt () throws IOException {
    // call next
}

To:
public static void main(String[] args)throws IOException{
    // call readint
    // call sum
}

static int readInt () throws IOException {
    // call next
}

static String next () throws IOException {
}

static int sum(int a, int b){
    //call isPrime
}

static boolean isPrime(int n){
}

The methods readInt and next can be replaced with java.util.Scanner:
Scanner scanner = new Scanner(System.in);
int q = scanner.nextInt();

Close the resources: br.close() (or scanner.close()) at the end of main or if exceptions occur.

PrintWriter pr is not used.

The method sum(int a, int b) does not sum two integers. A better name might be sumPrimesBetween

StringTokenizer is discouraged for new code, from the docs:

StringTokenizer is a legacy class that is retained for compatibility
reasons although its use is discouraged in new code. It is recommended
that anyone seeking this functionality use the split method of String
or the java.util.regex package instead.

Performance
As mentioned in the comments, since you know the upper bound 10^5 you can precompute all the prime numbers in advance. The main logic can be:

Precompute all prime numbers until 10^5: you can use an array of boolean initialized to true and then set the corresponding index to false if not prime. For example:

static void computePrimesUntil(int n) {
    isPrime = new boolean[n+1];
    Arrays.fill(isPrime, true);
    isPrime[0] = isPrime[1] = false;
    for (int i = 2; i <= n; i++) {
        if (isPrime[i] && (long) i * i <= n) {
            for (int j = i * i; j <= n; j += i){
                isPrime[j] = false;
            }
        }
    }
}

Code inspired from here.

Parse all the questions from the user
Output the sum of primes for each question

Note: depending on how the total running time is calculated, you need to consider the time to parse the input and print the results. The total running time also depends on the machine where you run the program.


Find the contiguous subarray within an array (containing at least one number) which has the largest sum Interview Q:  Find the contiguous subarray within an array (containing at least one number) which has the largest sum.

For example: Given the array [-2,1,-3,4,-1,2,1,-5,4],  the contiguous subarray [4,-1,2,1] has the largest sum = 6.

For this problem, return the maximum sum.

My solution (pseudocode and code) is below.  Works, but can someone tell me

Is there a faster algo?
Is the code structure done for an interview?

Pseudocode

//maxTillNow=A[0]
//maxStart=0
//maxLength=1
//Set currIndex =0
//Loop till currIndex == arrayLength
//  Set offset,sum=0
//      If A(currIndex] is -ve, then the only thing you need to do is check if it is > maxTillNow (in case all elements are -ve). If yes, set maxTillNow to A[currIndex] and move on

//      if +ve
//          Look at sums of elements starting at A[currIndex] ... A[currIndex+offset] for offsets in range 0 to currIndex+offset<length
//            if any sum > maxTillNow, store
//            Also find index of first -ve element you encounter as you look at elements A[currIndex+1]... -->firstNegIndex
//            nextElement to look at it firstNegIndex+1

Full code

int maxSubArray(const int* A, int n1) {
    int currIndex=0;
    int maxSumTillNow=A[0] ;
    int maxSumStart=0 ;
    int maxSumLength=1 ;
    int currLength;
    int sum ;
    int firstNeg ;

    while(currIndex<=n1-1)
    {
        if(A[currIndex]<0)
        {
            if(A[currIndex]>maxSumTillNow)
                maxSumTillNow=A[currIndex] ;
            currIndex++ ;
            continue ;
        }
        sum=0 ;
        firstNeg=-1 ;
        for(currLength=0;currLength+currIndex<=n1-1;currLength++)
        {
            sum+=A[currIndex+currLength] ;
            if(sum>maxSumTillNow)
            {
                maxSumTillNow=sum ;
                maxSumStart=currIndex ;
                maxSumLength=currLength+1 ;
            }
            if(firstNeg==-1 && A[currIndex+currLength]<0)
            {
                firstNeg=currIndex+currLength ;
            }
        }
        if(firstNeg==-1)
        {
            break ;
        }
        currIndex=firstNeg+1 ;
    }
    return maxSumTillNow ;

   }

I can also get additional info on exact sequence leading to max sum as below

    //    printf("Max sum is = %d, starting at index %d and length =%d\n",maxSumTillNow,maxSumStart,maxSumLength) ;
    //    printf("Max sub Array is [") ;
    //    for(currIndex=maxSumStart;currIndex<=maxSumStart+maxSumLength-1;currIndex++)
    //    {
    //        printf("%d,",A[currIndex]) ;
    //    }
    //    printf("]\n") ;

      }

1

The usual C coding conventions dictate that a space is added before and after a binary operator. For example, you should write

a += b;
if (foo < bar) ...
int currIndex = 0;

instead of

a+=b;
if(foo<bar) ...
int currIndex=0;

Also, you should add a single space before the opening parenthesis associated with keywords for, while, and if. For example, you should write

for (i = 0; i < x; ++i) ...

instead of

for(i = 0; i < x; ++i) ...

2

You add a space (sometimes) before the closing semicolon (;). Don't do it.

3

Your code would be more nifty if you added an empty line after each closing brace (}). For example,

if (sum > maxSumTillNow)
{
    ...
}

if (firstNeg == -1 && A[currIndex + currLength] < 0)
{
    ...
}

4

Your algorithm seems to run in quadratic time. The Kadane's algorithm does this in linear time.

Summa summarum

All in all, I had this in mind:

#include <stdio.h>
#define MAX(a, b) (a) > (b) ? (a) : (b)

int maxSubArray(const int* A, int n1)
{
    int currIndex     = 0;
    int maxSumTillNow = A[0];
    int maxSumStart   = 0;
    int maxSumLength  = 1;
    int currLength;
    int sum;
    int firstNeg;

    while (currIndex <= n1 - 1)
    {
        if(A[currIndex] < 0)
        {
            if(A[currIndex] > maxSumTillNow)
            {
                maxSumTillNow = A[currIndex];
            }

            currIndex++;
            continue;
        }

        sum = 0;
        firstNeg = -1;

        for (currLength = 0; currLength + currIndex <= n1 - 1; currLength++)
        {
            sum += A[currIndex+currLength];

            if (sum > maxSumTillNow)
            {
                maxSumTillNow = sum;
                maxSumStart   = currIndex;
                maxSumLength  = currLength + 1;
            }

            if (firstNeg == -1 && A[currIndex + currLength] < 0)
            {
                firstNeg = currIndex+currLength;
            }
        }

        if(firstNeg == -1)
        {
            break ;
        }

        currIndex = firstNeg + 1;
    }

    return maxSumTillNow;
}

int kadanesAlgorithm(const int* array, const size_t length)
{
    int maximum_so_far = array[0];
    int maximum_end    = array[0];

    for (size_t index = 1; index < length; ++index)
    {
        const int x    = array[index];
        maximum_end    = MAX(maximum_end + x, x);
        maximum_so_far = MAX(maximum_end, maximum_so_far);
    }

    return maximum_so_far;
}

int main(int argc, const char * argv[]) {
    int arr[] = { -2, 1, -3, 4, -1, 2, 1, -5, 4 };
    printf("Original result: %d\n", maxSubArray(arr, 9));
    printf("Kadane's result: %d\n", kadanesAlgorithm(arr, 9));
}

Hope that helps.


Find the first missing positive integer in an array of integers The problem is the one explained in Given an unsorted integer array, find the first missing positive integer

  Given an unsorted integer array, find the first missing positive
  integer.

  For example,
  Given [1,2,0] return 3,
  and [3,4,-1,1] return 2.

  Your algorithm should run in \$O(n)\$ time and use constant space.

I am trying to learn Java 8+ and wrote the following code. Request any improvisations possible.

int firstPositive(int[] inputArray) {

    //Convert all negative numbers to 0.
    IntStream.range(0, inputArray.length).forEach(idx -> {
        if (inputArray[idx] < 0) inputArray[idx] = 0;
    });

    IntStream.range(0, inputArray.length).forEach(idx -> {
        if (Math.abs(inputArray[idx]) == 0) {
            inputArray[0] = -Math.abs(inputArray[0]);
        }
        else if (Math.abs(inputArray[idx]) - 1 < inputArray.length && inputArray[Math.abs(inputArray[idx]) - 1] > 0) {
            inputArray[Math.abs(inputArray[idx]) - 1] *= -1;
        }
    });

    OptionalInt res = IntStream.range(0, inputArray.length).filter(idx -> inputArray[idx] >= 0).findFirst();

    if (res.isPresent()) {
        return res.getAsInt()+1;
    }

    return inputArray.length+1;
}

Instead of

if (res.isPresent()) {
    return res.getAsInt()+1;
}

return inputArray.length+1;

you could make use of the orElse method of OptionalInt:

return res.orElse(inputArray.length) + 1;