Search word in multiple files I am writing a brute-force method to search for a word. Please review my code and tell me how I can improve it.

public class Searcher {

  private static String filepath = null;
  private static final String defaultPath = "//sample_text";

  private Searcher() {
    this.filepath = defaultPath;
  }

  private Searcher(String dir) {
    if (dir.isEmpty()) {
      this.filepath = defaultPath;
    } else {
      this.filepath = dir;
    }
  }

  public static void main(String[] args) throws IOException {
    Searcher search = new Searcher();

    String folderToSearch = search.filepath;

    File folder = new File(folderToSearch);
    Set<File> list = new HashSet<File>();
    search.getFiles(folder, list);

    String toSearch = "The";
    for (File file : list) {
     BruteForceSearch bSerch = new BruteForceSearch(file);
     bSerch.search(toSearch);
    }
  }

  private void getFiles(File folder, Set<File> list) {

    folder.setReadOnly();
    File[] files = folder.listFiles();
    for (int j = 0; j < files.length; j++) {
      list.add(files[j]);
      if (files[j].isDirectory())
        getFiles(files[j], list);
    }
  }

}

public class BruteForceSearch implements WordSearch{

  private final String fileName;
  private Map<String, Integer> result =  new HashMap<String, Integer>();
  private File file;
  private int count =0;

  public BruteForceSearch(File file) throws IOException {
    this.file =  file;
    this.fileName = file.getName();
    this.count = 0;
  }

  private void searchBruteForce(String toSearch) throws IOException {

      FileInputStream fstream = new FileInputStream(file);
      BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
      String readLine = "";
      while ((readLine = in.readLine()) != null) {
        String[] words = readLine.split("\\W");
        for (String text : words) {
          if (text.equalsIgnoreCase(toSearch)) {
            count++;
          }
        }
      }
      in.close();
  }

  public String getFile() {
    return fileName;
  }

  public int getCount(String word) {
    return count;
  }

  public void search(String toSearch) throws IOException {
    searchBruteForce(toSearch);
    if (getCount(toSearch) != 0) {
        System.out.println(getFile() + " - " + getCount(toSearch) + " matches ");
    }
  }
}
public interface WordSearch {

  String getFile();
  int getCount(String term);
}

Here the interface is used for other search methods as well (like using regex, using index based). Brute-force is one such implementation.

Semantic of methods

The implementation or the signature of the method "int getCount(String term);" does not make sense. Either you delegate the parameter "term" to the search algorithm with a stateless implementation or you omit it and have a stateful implementation. In the last case your "term" should be given to the constructor. I prefer a stateless implementation and all further comments are related to that.

Programming using interfaces

You are not using the interface "WordSearch". The assigment

BruteForceSearch bSerch = new BruteForceSearch(file);

be to the abstraction:

WordSearch search = new BruteForceSearch(file);

Avoid passing working list references

The code

Set<File> list = new HashSet<File>();
search.getFiles(folder, list);

should be refactored to:

Set<File> list = search.getFiles(folder);

The "List" should be instantiated within the method "getFiles(...)".

Make directory recursion part of WordSearch through template pattern

Currently you are collecting all files recursively within the main-method. Make the algorithm part of the WordSearch objects. To do so, refactor the WordSearch-Interface to an abstract class and implement the directory recursion as a Template Method.

public abstract class WordSearch {

    ...

    public int getCount(String term) {
        File folder = ...;
        return getCountInFolder(folder);
    }

    /**
     * Template method pattern
     */
    protected abstract int getCountInFile(File file, String term);

    private int getCountInFolder(File folder, String term) {
        int count = 0;
        File[] files = folder.listFiles();
        for (int j = 0; j < files.length; j++) {
            if (files[j].isDirectory()) {
                count = count + getCountInFolder(files[j], term);
            } else {
                count = count + getCountInFile(files[j], term);
            }
        }
        return count;
    }

    ...

}

Introduce strategy pattern

Associate a WordSearch-Object within "Searcher" and delegate to it. This is called Strategy pattern.

public class Searcher {

    private WordSearch wordSearch;

    public Searcher(String dir, WordSearch wordSearch) {
        this.wordSearch = wordSearch;
        ...
    }

    public int getCount(String term) {
        return this.wordSearch.getCount(term);
    }

    public void setWordSearch(WordSearch wordSearch) {
        this.wordSearch = wordSearch;
    }

    ...

}

The WordSearch-Object may be set through the constructor and changed by a setter.


Simple DFA using the State Monad I've been getting to grips with some Monads in Haskell recently, and to get some practice I plucked a Mealy Machine out of thin air and decided to implement it using the State Monad (it's a simple machine and a simple implementation, don't worry).

The input is a string of characters from +, -, n, e.

+, "plus". Output a 1,
-, "minus". Output a -1.
n, "toggle negated". Until "toggled off", flip the sign of the output from + or -.
e, "toggle enabled". Until "toggled back on", output 0 from + or -.

The output is just a sequence of the same length as the input, containing 1, 0, and -1s as outputted by the DFA. In the code below, the output is actually the sum of this sequence, acting as a quick checksum. All states are accepting states.

For example, an input of +-n+-e+e+ would result in [1, -1, 0, -1, 1, 0, 0, 0, -1].

To unambiguously define the behaviour, here's a pretty messy state transition diagram:

If "posate" (as the opposite of negate) isn't a word already, it is now!
The transitions are labelled with the "english name", the symbol used to represent this transition, and the "output" from the machine. All states are accepting states, I left out the conventional double rings.

At last, the code:

import Control.Monad.Trans.State.Lazy
import Control.Monad (foldM)

data Input = Plus | Minus | ToggleNegate | ToggleEnabled
type Emission = Integer
type Accum = [Emission]
type Output = [Emission]
type Negated = Bool
type Enabled = Bool

toInput :: Char -> Input
toInput '+' = Plus
toInput '-' = Minus
toInput 'n' = ToggleNegate
toInput 'e' = ToggleEnabled
toInput  _  = error "Invalid input representation"

-- Determine new state of state machine along with transition emission
step :: (Negated, Enabled, Input) -> (Negated, Enabled, Emission)
step (n, e, ToggleNegate)  = (not n, e, 0)
step (n, e, ToggleEnabled) = (n, not e, 0)
step (n, False, i)         = (n, False, 0)
step (n, e, Plus)          = (n, e, if n then -1 else  1)
step (n, e, Minus)         = (n, e, if n then  1 else -1)

-- Helper function for "evaluate"'s foldM
mapEmissions :: Accum -> Input -> State (Negated, Enabled) Output
mapEmissions accum input = do
    (negated, enabled) <- get
    let (negated', enabled', emission) = step (negated, enabled, input)
    put (negated', enabled')
    return (accum ++ [emission])

-- Process an input string and return the result
-- (False, True) is the start state: (not negated, enabled)
evaluate :: [Input] -> Output
evaluate inputs = evalState (foldM mapEmissions [] inputs) (False, True)

-- Convenience function for output formatting
shouldEqual :: String -> Integer -> IO ()
shouldEqual input expected = do
    let actual = (sum . evaluate . map toInput) input
    putStrLn $ "Expected " ++ show expected ++ ", got " ++ show actual ++ ": " ++ input

main :: IO ()
main = do
    "+-n--n" `shouldEqual` 2
    "+e----e++" `shouldEqual` 3
    "-n++e++e--+-n++" `shouldEqual` 1

Code online here.

I'm happy to hear any and all critiques and advice, but in particular:

Am I using the State monad idiomatically? Can I write any bits of code more elegantly?
Is this an appropriate use for this monad? This isn't a particularly complex task, but I feel like it makes passing around the DFA's state simpler.

Thanks for taking the time to read, and Happy New Year!

Use traverse instead of foldM + accumulate

In a fold, you usually use a function that reduces (or folds) your input into a single result. You, on the other hand, use fold mapEmissions to create a list from an input list. Both lists will have the same number of elements. That's not a fold. That's a map.

So first of all, let's use traverse instead of foldM:

evaluate :: [Input] -> Output
evaluate inputs = evalState (traverse emit inputs) (False, True)

Next, let's figure out how emit should look like:

emit :: Input -> State (Negated, Enabled) Emission
emit input = do
    (negated, enabled) <- get
    let (negated', enabled', emission) = step (negated, enabled, input)
    put (negated', enabled')
    return emission

Almost the same as mapEmission, and we can use emit to implement mapEmission:

mapEmission accum input = (accum ++) <$> emit input

Therefore, we should really replace the foldM by traverse.

Consider already existing interfaces

step can be rewritten with another interface that fits the state interface:

type MMState = (Negated, Enabled) --  Mealy Machine State

step :: Input -> MMState -> (MMState, Emission)
step ToggleNegate  (n, e)     = ((not n, e    ), 0)
step ToggleEnabled (n, e)     = ((    n, not e), 0)
step _             (n, False) = ((n,     False), 0)
step Plus          (n, e)     = ((n,     e    ), if n then -1 else  1)
step Minus         (n, e)     = ((n,     e    ), if n then  1 else -1)

This makes emit even easier:

emit :: Input -> State MMState Emission
emit input = state (step input)

Note that MMState makes it easier to change the other types, as there is less repetition.

It's completely up to you to write step like this, though.

Encapsulate possible errors in the type (if possible)

In toInput consider Maybe Input or Either String Input instead of error:

toInput :: Char -> Either String Input
toInput '+' = pure Plus
toInput '-' = pure Minus
toInput 'n' = pure ToggleNegate
toInput 'e' = pure ToggleEnabled
toInput  c  = Left $ "Unrecognised character: " ++ [c]

Use Int instead of Integer if the values are small

Instead of Integer we should use Int as Emission. Integer can be arbitrarily large, but your output values are either -1, 0 or 1. Int is much more suitable for that.

Other comments

Other than that, well done. You followed good practice by including type signatures, and your code is easy to read and to follow. The only real hurdle was foldM instead of traverse.

However, your single instruction type is called Input, whereas your complete output type is called Output. That naming is somewhat unbalanced.

Overall, we end up with:

import Control.Monad.Trans.State.Lazy

data Input = Plus | Minus | ToggleNegate | ToggleEnabled
type Emission = Int
type Output = [Emission]
type Negated = Bool
type Enabled = Bool
type MMState = (Negated, Enabled) --  Mealy Machine State

toInput :: Char -> Either String Input
toInput '+' = pure Plus
toInput '-' = pure Minus
toInput 'n' = pure ToggleNegate
toInput 'e' = pure ToggleEnabled
toInput  c  = Left $ "Unrecognised character: " ++ [c]

step :: Input -> MMState -> (MMState, Emission)
step ToggleNegate  (n, e)     = ((not n, e),     0)
step ToggleEnabled (n, e)     = ((    n, not e), 0)
step _             (n, False) = ((n,     False), 0)
step Plus          (n, e)     = ((n,     e),     if n then -1 else  1)
step Minus         (n, e)     = ((n,     e),     if n then  1 else -1)

emit :: Input -> State MMState Emission
emit input = state (step input)

evaluate :: [Input] -> Output
evaluate inputs = evalState (traverse emit inputs) (False, True)

By the way, we can now use emit on any traversable, so we could relax evaluate's type to

evaluate :: Traversable t => t Input -> t Emission
evaluate inputs = evalState (traverse emit inputs) (False, True)

But that's up to you.


Write log file in nodeJS with setInterval I wrote this code to write the log file every 5 seconds to avoid excessive disk I/O (which can cause HDD temperature to increase or SSD to wear off):

var fs = require('fs');
var logText = '';
function log(){
    fs.appendFile('c:/log.txt', logText, (err) => {
        if (err) throw err;
        logText = '';
    });
}
setInterval(log, 5000);

Is this the correct approach? I mean this is async right? Will setInterval interfere with the performance?

I'm assuming that you are changing the logText variable to control what's being logged. If you want to keep this same interface, the only optimization that I can see is to make log() check if logText is not empty before it writes it:

var fs = require('fs');
var logText = '';
function writelog(){
    if (logText) {
        fs.appendFile('c:/log.txt', logText, (err) => {
            if (err) throw err;
            logText = '';
        });
    }
}
setInterval(writelog, 5000);

I would also add a function that adds to the log text:

function log(text){
    logtext += ("Log prefix stuff here" + text)
}


Determines whether a number is a power of 2 The task:

Write a method that takes in a number and returns true if it is a power of 2. Otherwise, return false.
You may want to use the % modulo operation. 5 % 2 returns the remainder when dividing 5 by 2; therefore, 5 % 2 == 1. In the case of 6 % 2, since 2 evenly divides 6 with no remainder, 6 % 2 == 0."

Model solution:

def is_power_of_two?(num)
  if num < 1
    return false
  end

  while true
    if num == 1
      return true
    elsif num % 2 == 0
      num = num / 2
    else
      return false
    end
  end
end

My solution:
def is_power_of_two?(num)

0.upto(num) do |i|
  return true if 2**i == num
  return false if 2**i > num
end
end

This is a problem from a beginner's course. I'm trying to return to these problems now that I've learned some more idiomatic Ruby.
Generally, are there any advantages to the model solution? Specifically, how could I improve my solution?

Both of these ultimately do a linear search trying one power of two after another to see if that one is equal to the input.

Depending on your intent, there's a way that might be better.

A power of two has exactly one bit set, so our job is really to determine whether only one bit is set in the number.

Let's consider a number with only one bit set: 000010000. If we subtract one from this number, we get: 000001111--that is, the least significant 0 bits in the number now become ones, and the least significant bit that was set becomes a 0.

Now, lets see what happens if we do the same to a number with two or more bits set. Let's consider 00010100. If we decrement it, we get 00010011. The same thing has happened--the least significant bit that was set has become a zero, the less significant bits have become one, and any more significant bits remain unchanged.

So, we can define our function as:

def is_power_of_two?(num)
    return num & (num - 1) == 0
end

Sumary

Pros

This has the advantage of being short, simple, and relatively fast.

Cons

It has the disadvantage that although the code is readable, many (especially less experienced programmers) may find the algorithm difficult to understand.


Invoking thread for each item in list simultaneously and returning value in Java I made a wrapper for an API that I am using, basically what I'm doing is sending in a list of Series objects and for each item in the list, a thread ("worker"?) starts and fetches information for their respective series.

The API that I'm using doesn't support fetching several series using one request, which would simplify this by a large margin, therefore I run threads for each item in the list.

I'm mostly concerned about the thread part, but I'll provide all the other parts as well.

@RequestMapping(value = "/series", method = RequestMethod.POST)
public List<Response> fetchSeveralSeries(@RequestBody List<Series> list) throws InterruptedException, ExecutionException {
    List<Callable<Response>> callableList = new ArrayList<>();
    for (int i = 0; i < list.size(); i++) {
        callableList.add(new performRequest(list.get(i).getSeries()));
    }

    final ExecutorService service = Executors.newFixedThreadPool(list.size());
    List<Future<Response>> futureObjects = service.invokeAll(callableList);
    List<Response> series = new ArrayList<>();

    for (Future<Response> obj : futureObjects) {
        series.add(obj.get());
    }

    return series;
}

Callable class used. (temporarily in my Spring boot controller class, as my entire implementation is in the controller atm.)

private class performRequest implements Callable<Response> {
    private int seriesId;

    private performRequest(int seriesId) {
        this.seriesId = seriesId;
    }

    @Override
    public Response call() throws UnirestException {
        return request.send(new Url(seriesId).toString());
    }
}

About the request made by each thread, I have a Url class which constructs the Url for the request and a Request class which sends the request to the external API.

Request class which sends the request to external API. The Response object is a "GSON Object".

@Service
public class Request {

    private static final Logger logger = LoggerFactory.getLogger(Request.class);

    public Response send(String url) throws UnirestException {
        logger.info("sending GET");
        HttpResponse<JsonNode> json = Unirest.get(url).asJson();
        String body = json.getBody().toString();
        return new Gson().fromJson(body, Response.class);
    }

}

Url class which constructs the URL.

@Data
public class Url {
    private static final String BASE_URL = "https://api.themoviedb.org/3";
    private static final String API_KEY = System.getenv("API_KEY");

    private Integer id;
    private Integer season;
    private Integer episode;
    private String searchQuery; // should turn spaces into %20

    public Url(String searchQuery) {
        this.searchQuery = searchQuery;
    }

    public Url(Integer id) {
        this.id = id;
    }

    public Url(Integer id, Integer season) {
        this.id = id;
        this.season = season;
    }

    public Url(Integer id, Integer season, Integer episode) {
        this.id = id;
        this.season = season;
        this.episode = episode;
    }

    @Override
    public String toString() {
        return this.searchQuery != null ?
                String.format("%s/search/tv?api_key=%s&language=en-US&query=%s", BASE_URL, API_KEY, this.searchQuery) : this.episode != null ?
                String.format("%s/tv/%s/season/%s/episode/%s?api_key=%s&language=en-US", BASE_URL, this.id, this.season, this.episode, API_KEY) : this.season != null ?
                String.format("%s/tv/%s/season/%s?api_key=%s&language=en-US", BASE_URL, this.id, this.season, API_KEY) :
                String.format("%s/tv/%s?api_key=%s&language=en-US", BASE_URL, this.id, API_KEY);
    }

}

An example POST request body would be something like below:

[
    {
        "series": 37680
    },
    {
        "series": 53123
    },
    {
        "series": 12345
    },
    {
        "series": 29182
    }
]

Could my code be improved somehow? Would it be possible to decrease response time? Is it "thread-safe"?
  Any feedback is welcome.

Consider following point:

For Request.send() is thread safe as it does not used any sharing resource of Request class.
As per provided code single(singleton) object will be created as per default spring configuration.
You must shutdown ExecutorService once no more thread need to be add.


Ruby: refactor simple string method for aligning DSV text GOAL:

Accept DSV strings where the delimiter may consist of more than one character
Accept DSV strings where there are no embedded delimiters
Output text with no modification to the source string other than aligning custom delimiter with optional padding

QUESTION:

Anyone have a re-factor or stylistic enhancement or two?

CODE:

module DreftymacAddon
  module String
    ### <region-fdef ddef="dsvpretty ;; lineup delims pretty print dsv text with lined up delimiters ">
    def dsvpretty(delim=';;',padd='')
      sout    =   ""
      asizes  =   []
      alines  =   self.to_s.split(/[\x0a\x0d]/)
      alines.each{|currline|
        currline.split(delim).each_with_index{|currfld,icc|
          asizes[icc] = [asizes[icc].to_i,currfld.length].max()
        }
      }
      alines.each{|currline|
        next if currline.strip == ''
        atemp   = []
        currline.split(delim).each_with_index{|currfld,icc|
          atemp << sprintf("%-#{asizes[icc]}s", currfld)
        }
        sout += atemp.join([padd , delim , padd].join(''))
        sout += "\n"
      }
      sout
    end
    alias :dsv_pretty :dsvpretty
    ### </region-fdef>
  end
end

class String
  include DreftymacAddon::String
end

### String.new().dsvpretty ;; example usage
##{
if(1000!=0);
  vout   = %Q[
    LNAME|FNAME|AMOUNT|AGE|NATION|PLATFORM
    Rodriguez|Johan|30.00|58|uk|windows_xp
    Raynor|Coty|40.00|14|uk|redhat
    Bruen|Jackie|30.00|17|uk|ubuntu
    Breitenberg|Waldo|20.00|14|canada|mac_osx
    Collier|Lucy|30.00|22|canada|windows_vista
    Blick|Emmett|30.00|59|canada|knoppix
    Schroeder|Myrtis|10.00|29|uk|knoppix
    Rodriguez|Ashtyn|20.00|22|uk|mac_osx
    Leuschke|Sigmund|40.00|21|france|redhat
    Fahey|Cassidy|40.00|29|canada|knoppix
  ]

  puts vout.dsvpretty('|','')
=begin
    LNAME      |FNAME  |AMOUNT|AGE|NATION|PLATFORM
    Rodriguez  |Johan  |30.00 |58 |uk    |windows_xp
    Raynor     |Coty   |40.00 |14 |uk    |redhat
    Bruen      |Jackie |30.00 |17 |uk    |ubuntu
    Breitenberg|Waldo  |20.00 |14 |canada|mac_osx
    Collier    |Lucy   |30.00 |22 |canada|windows_vista
    Blick      |Emmett |30.00 |59 |canada|knoppix
    Schroeder  |Myrtis |10.00 |29 |uk    |knoppix
    Rodriguez  |Ashtyn |20.00 |22 |uk    |mac_osx
    Leuschke   |Sigmund|40.00 |21 |france|redhat
    Fahey      |Cassidy|40.00 |29 |canada|knoppix
=end

  puts vout.dsvpretty('|',' ')
=begin
    LNAME       | FNAME   | AMOUNT | AGE | NATION | PLATFORM
    Rodriguez   | Johan   | 30.00  | 58  | uk     | windows_xp
    Raynor      | Coty    | 40.00  | 14  | uk     | redhat
    Bruen       | Jackie  | 30.00  | 17  | uk     | ubuntu
    Breitenberg | Waldo   | 20.00  | 14  | canada | mac_osx
    Collier     | Lucy    | 30.00  | 22  | canada | windows_vista
    Blick       | Emmett  | 30.00  | 59  | canada | knoppix
    Schroeder   | Myrtis  | 10.00  | 29  | uk     | knoppix
    Rodriguez   | Ashtyn  | 20.00  | 22  | uk     | mac_osx
    Leuschke    | Sigmund | 40.00  | 21  | france | redhat
    Fahey       | Cassidy | 40.00  | 29  | canada | knoppix
=end

end
##}

Some notes:

include DreftymacAddon::String. Monkeypatching common classes is always controversial, but in this case I wouldn't do it, it's not a general enough abstraction.
As always, I'd recommend using functional style with Ruby (if you check my other answers in this site, it's all I do ;-)) unless you have a very good reason not to. No sout = "", asize = [], each, array[key] = value, += and other statements, use expressions.
dsvpretty(delim=';;',padd=''). This kind of signature is problematic, if you want to set padd you are forced to set also delim when you wanted to keep the default value. Use an options hash instead. Note that finally Ruby 2.0 provides keyword arguments.
each_with_index{|currfld,icc|. Let the block breathe, and don't forget spaces after commas: each_with_index { |currfld, icc|. More on idiomatic Ruby.

I'd write:

module DreftymacAddon
  def self.dsv_pretty(string, options = {})
    options = {:delim => ";", :padding => ""}.merge(options)
    rows = string.strip.split(/\n/).map { |line| line.strip.split(options[:delim]) }
    columns_max_width = rows.map { |row| row.map(&:size) }.transpose.map(&:max)
    columns_separator = options[:padding] + options[:delim] + options[:padding]

    rows.map do |row|
      row.zip(columns_max_width).map do |cell, column_max_width|
        cell.ljust(column_max_width)
      end.join(columns_separator)
    end.join("\n")
  end
end

puts DreftymacAddon::dsv_pretty(vout, :delim => '|', :padding => " ")


How to better organize the declarations of these functions? So as I understand it, you should always declare a function before it gets called.

However in the code below, how could this be restructured so that this philosophy is satisfied?

fetchTags is declared here as the 3rd function, somewhere below here I call it from a loadTags function.

After the tags are loaded, I then attached the EventListener and attach the 2nd function scrollingTags.

Once scrollingTags hits bottom and calls loadMoreItems which returns checkScrollLimit and if this !LIMIT_REACHED is true, limit is not reached, it calls fetchTags again which is declared below checkScrollLimit.

My code works so maybe this is a non-issue, just curious how other developers would treat this.

const checkScrollLimit = () => {
    if (!LIMIT_REACHED) {
        const infiniteScroll = true;
        PARAMS.start += this.limit;
        maxlimitCheck() ? LIMIT_REACHED = true : fetchTags(infiniteScroll);
    }
};

const scrollingTags = () => {
    const reachedBottom = () => tagsCol.scrollHeight - tagsCol.scrollTop === tagsCol.offsetHeight;
    const loadMoreItems = () => checkScrollLimit();
    if (reachedBottom()) loadMoreItems();
};

const fetchTags = (infiniteScroll) => {
    LIMIT_REACHED = false;
    return GetTagsFactory.getTags(false, Util.encodeTicker(this.ticker), PARAMS).then((tags) => {
        TOTAL_TAGS = tags.total;
        this.tags = fillTagsList(tags, infiniteScroll);
        tagsCol.addEventListener('scroll', scrollingTags);
        return this.tags;
    });
};

Structure's ok. The functions are all tightly coupled, which may make changes more of a hassle, but it's nothing super terrible.

It could be cleaner if you separated concerns a bit more. E.g. fetchTags would do only that: Fetch tags, but not append them to the page or anything. And checkScrollLimit would do only that, and just return a boolean instead of calling other functions.

Having fetchTags also append stuff to the page is what leads to attaching the event listener, which causes the circular dependency with scrollingTags, which in turn is coupled to checkScrollLimit which is coupled to fetch.... - you get the idea.

So you could add higher-order functions - say, something like an appendTags function which calls fetchTags and fillTagsList and finally adds the event listener. Then fetchTags wouldn't be coupled to scrollingTags and vice versa; a higher-order function(s) would in charge to tying all the ends together.

Still, I'd say it's mostly a non-issue in this particular case. All your functions are defined before they're called, and that's what matters. The JS runtime doesn't care about the order of the function definitions, only that they have been defined before something tries to call them. So, really, you could mix them up however you want.

Sidenote: The scrollingTags function seems a little too complicated, though, what with wrapping checkScrollLimit in another local function. Could just skip that and call it directly.

On a related note, just your edification: If you had used "proper" function declarations - i.e. function foo() {...} – JavaScript would "hoist" those declarations and interpret them before starting the rest of the script evaluation. So in that case, the code would work even if you actually called a function before it had (seemingly) been defined. So, curiously, this works:

foo();

function foo() { ... } // hoisted; evaluated before anything else

but this doesn't:

foo(); // error: foo isn't defined (yet)

var foo = function () { ... } // not hoisted

and nor do the ES6 => declarations.


Convert an integer(seconds) into the format of an hour: minute: seconds in c input:140153(seconds)
output:38:55:53

I have solved this problem in this way:
#include <stdio.h>
int main(){
    int N;
    double hour;
    double minute;
    double seconds;
    scanf("%d",&N);

    hour=N / 3600.0;

    double floating = hour;
    double fractional, integer;

    //split the floating value(hour) into the integer and decimal
    fractional = modf(floating, &integer);
    minute=60 * fractional;

    double floatingUpdate = minute;
    double fractionalUpdate, integerUpdate;
    //split the floating value(minute) into the integer and decimal
    fractionalUpdate = modf(floatingUpdate,&integerUpdate);
    seconds = 60 * fractionalUpdate;
    printf ("%g:%g:%g\n", integer, integerUpdate, seconds);

    return 0;
}

Would you propose any simplification for my solution? Would I face any problem with some specific conditions? Is the way I solved this kind of problem efficient or not?

When we write

scanf("%d",&N);

it's important not to ignore the return value from scanf() (that indicates how many values were successfully converted).  Otherwise, we're working with the uninitialised value N.

Use of the modf() function requires a declaration:
#include <math.h>

But we would be better working with integers instead (use the / and % operators).

Presentation of negative values is strange:

-70:-29:-29

Normally, we'd just print that as -70:29:29.
I'd be inclined to just work with unsigned values, and get that working well, before extending to negatives.

Variable naming is inconsistent (singular hour and minute, but plural seconds).  N is a poor choice for a variable name - choose something more meaningful and lower-case; convention reserves upper-case for macros.

We should make the declaration of main() be a prototype.  Also, it's permitted to omit the return statement in main(); since we always return success, let's remove that clutter.

Modified code
#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    unsigned long total_secs;
    while (scanf("%lu", &total_secs) != 1) {
        // consume some input
        if (scanf("%*s") == EOF) {
            return EXIT_FAILURE;
        }
        printf("Enter a valid integer: ");
        fflush(stdout);
    }

    unsigned long hour = total_secs / 3600;
    unsigned int minute = total_secs / 60 % 60;
    unsigned int second = total_secs % 60;

    printf("%lu:%u:%u\n", hour, minute, second);
}


LeetCode heater binary search solution (w recursion) optimisation I was doing LeetCode heater problem.
Question: https://leetcode.com/problems/heaters/

Winter is coming! Your first job during the contest is to design a standard heater with fixed warm radius to warm all the houses.
Now, you are given positions of houses and heaters on a horizontal line, find out minimum radius of heaters so that all houses could be covered by those heaters.
So, your input will be the positions of houses and heaters seperately, and your expected output will be the minimum radius standard of heaters.

Example Input
Input: [1,2,3],[2]
Output: 1
Explanation: The only heater was placed in the position 2, and if we use the radius 1 standard, then all the houses can be warmed.

The solution I wrote is accepted by leetcode but it isn't optimal

Runtime: 656 ms, faster than 39.02% of JavaScript online submissions
for Heaters. Memory Usage: 48.2 MB, less than 12.20% of JavaScript
online submissions for Heaters.

My solution with comments
function findClosest (house, heaters) {
// if only one heater in array of heaters, subtract it with house number to get the difference and return it
if (heaters.length === 1) return Math.abs(house - heaters[0])
  const middleIndex =  Math.floor((heaters.length - 1)/2)
  // if middle heater is equal to house, heater exist on that house number, difference would be zero
  if (house === heaters[middleIndex]) return 0
  // heater on the leftside and rightside of the middle Heater, if leftside and rightside does not contain any elements (undefinned) then middle would be the left and right most element
  const left = heaters[middleIndex - 1] || heaters[middleIndex]
  const right = heaters[middleIndex + 1] || heaters[middleIndex]
  // if the left side heater location is greater than current house location, we need to move to left
  if (left > house) {
   return findClosest(house, heaters.slice(0, middleIndex+1))
  }
    // if the right side heater is less than current house, we need to move to right
  if (house>right) {
    return findClosest(house, heaters.slice(middleIndex + 1, heaters.length))
  }
 // finding diff b/w left, right and middle index and returning the ones with lease distance
  const middleDiff = Math.abs(house-heaters[middleIndex])
  const leftDiff = house - left
  const rightDiff = right - house
  return Math.min(middleDiff, leftDiff, rightDiff)
}

function findRadius  (houses, heater) {
  let maxIndex = 0
  houses.sort((a,b) => a-b)
  heater.sort((a,b) => a-b)
  for (let i=0; i<houses.length; i++) {
    const currentIndex = findClosest(houses[i], heater)
    if (currentIndex > maxIndex) maxIndex = currentIndex // if the current returned distance is the highest, set that as maxIndex
  }
  return maxIndex
}

Can someone help me in optimising it?

Performance The main optimization that can be made here is to, while iterating over the sorted houses, to inch along the heaters one-by-one while testing their distance. Increment a heater index only if the difference between the house and the next heater is less than the difference between the house and the current heater. This way, whenever a heater is tested, if the distance between it and the current house has become too great, that heater never gets tested again.
For example, when iterating over 5 houses and 5 heaters, the persistent indicies for each collection being used by the algorithm could look like this:
Indicies being examined:
houses heaters
0      0
1      0
2      0
2      1   // at this point, heater 1 is found to be closer to house 2 than heater 0
2      2   // heater 2 is found to be closer to house 2 than heater 1
3      2
4      2
4      3
4      4
5      4
5      5

And that's it. And, of course, on every iteration, check the distance difference between the heater and the house, and call Math.max on that difference and the current distance record to get the new distance record.
This approach reduces the overall computational complexity from O(n log m) (a binary search for every house) to O(n + m).
function findRadius(houses, heaters) {
    houses.sort((a, b) => a - b);
    heaters.sort((a, b) => a - b);
    // Persistent variable of the current closest heater to the house being iterated over
    let closestHeater = heaters[0];
    // and its index
    let closestHeaterIndex = 0;
    // Current record of needed heater radius
    let requiredRadius = 0;

    for (let i = 0, len = houses.length; i < len; i++) {
        let lastDiff = Math.abs(houses[i] - closestHeater);
        // If we aren't at the end of the heater array,
        // test the next heater to see if we need to increment the heater index:
        while (heaters[closestHeaterIndex + 1]) {
            const nextDiff = Math.abs(heaters[closestHeaterIndex + 1] - houses[i]);
            if (nextDiff > lastDiff) break;
            // This heater is closer to houses[i] than the current one in closestHeater
            lastDiff = nextDiff;
            closestHeaterIndex++;
            closestHeater = heaters[closestHeaterIndex];
        }
        requiredRadius = Math.max(requiredRadius, lastDiff);
    }
    return requiredRadius;
}

Result (screenshot):

Runtime: 88 ms, faster than 98.89% of JavaScript online submissions for Heaters.

Memory Usage: 42.6 MB, less than 5.56% of JavaScript online submissions for Heaters.

A couple other notes regarding your code:
Save array length If performance is an issue (it rarely is except in these competitions), you can save the current length of the array in a for loop in a variable to save a bit of calculations, like I did above:
for (let i = 0, len = houses.length; i < len; i++) {

Branches are expensive Logical branches like if and else are often required, but keep in mind that they generally slow things down more than most other operations:

As a general rule of thumb, branches are slower than straight-line code (on all CPUs, and with all programming languages). -jmrk, V8 developer

If you have a lot of branches, your code may not be as fast as you'd want it to be. Reduce them when possible if performance really is an issue.
Braced blocks create environments - or environment records, which are internal mappings of identifiers to their variable names. If you follow the specification exactly, having such blocks results in a bit more operations being performed. I'm not sure if it's an issue with modern engines, or if they can optimize it away completely, but leaving off { and }s when the block contains only a single statement might make things a smidgen faster. (But on the other hand, leaving off braces arguably makes the code a bit harder to read...)
Use representative variable names You defined the function as
function findRadius  (houses, heater) {

but heater is a collection of heaters; seeing heater.sort((a,b) => a-b) confused me at first glance. Best to name collections something plural, probably.


Templating libraries' intelligibility and their performance (compared to mine) I've just started writing my own little templating library in JavaScript, because as I went through others, there is always this voice in head, which says: "Oh, this is a lot of code and functionality. Is that really necessary or would it slow down the performance?"

If you are able to follow conventions, you would probably run into less issues.

So I ended up with this little script with the upper quote in my head.  The convention, here is that data needs an object and its keys must be the same like in template, mentioned between those delimiter-brackets "{{key}}"

function Template(url, data) {

    this.url = url;
    this.data = data;

    this.delimiter = ['{{','}}'];

    this.load = function() {

        var string = new String();

        var http = null;
        if ( window.XMLHttpRequest ) {
            http = new XMLHttpRequest();
        } else {
            http = new ActiveXObject("Microsoft.XMLHTTP");
        }

        http.open('GET', url, false);
        http.onreadystatechange = function() {
            if ( http.readyState === 4 ) {
                string = http.response;
            }
        };
        http.send(null);

        this.template = string;
        return this;

    };

    this.exchange = function() {

        for ( var key in this.data ) {

            var bracketedKey = this.delimiter[0] + key + this.delimiter[1];
            var indexOfKey = this.template.indexOf(bracketedKey);
            var lengthOfKey = bracketedKey.length;

            var exchange = this.template.substring(indexOfKey, indexOfKey+lengthOfKey);

            this.template = this.template.replace(exchange, this.data[key]);

        }

        return this;

    };

    this.build = function(selector) {

        this.load();
        this.exchange();

        var div = document.createElement('div');
        div.innerHTML = this.template;
        div = div.firstChild;

        if ( typeof selector === 'string' ) {
            document.querySelector(selector).appendChild(div);
        } else {
            return div;
        }

    };

}

But now I've started thinking about the fact, that other libraries include more code than mine and there must be a reason for it, mustn't it?

There also a few more things that are coming to my head, like:

Less code = better performance?
Why do so many people relate their projects with this templating library?
(Matter of performance => that would refute my first point)

It'd be cool if someone could explain me a few things about this topic.

Am I right with my mindset?
When I am right, is there something that I could improve, or even when I am wrong?
If you would recommend a library to me, what is your reason?

... as I went through others, there is always this voice in head, which says: "Oh, this is a lot of code and functionality. Is that really necessary or would it slow down the performance?"

This is a reasonable consideration, but premature optimisation is evil for a reason.   It might be a good idea to try to roll out your own solution, but it would be beneficial to learn more about the problem and other existing solutions before implementing it yourself.
As for minimalistic templating library, take a look at Underscore _.template.
It's literally 45 lines (add one or two helper functions) and does its job well.
As for your solution, I have a few points:
Don't use AJAX to load templates
If you're worried about performance, loading templates via AJAX is a bad idea. This will be a terrible bottleneck, not some hundred more lines of code in a different templating library.
Moreover, your code assumes you need to make an HTTP request each time you render something. If you have a list view with 20 items, that would be 20 requests.
This is absolutely unsuitable for production code.
Don't use synchronous AJAX calls, like, ever
The very point of AJAX is processing things asynchronously, without blocking other scripts from executing. There is rarely a reason to do things synchronously.
Again, please don't do this in production code.
Instead, put templates in HTML or (better) compile them to JS
Most, if not all, templating libraries assume the template is already available in the client code, and this is the correct way to go. Now, there are two ways how you can accomplish this:
1. You can embed templates in DOM with tricks like


Using an alternative for a switch/state-based custom parser I'm currently developing a small tool that applies a number of regex-based match/replacement-pairs to the contents of a file (or, more generally speaking, a string).
Note that regex-knowledge is not relevant to this answer, which is the reason why I didn't add it to the tags.

Now, this tool also supports saving all of your match/replace-pairs to a file. Since I've made bad experiences when using GSON with lists or arrays in the past, I decided to use my own format - considering it just has to save some kind of match-replace-separator-match-replace-separator-etc-style structure.

Now, my code works so far, but during refactoring, I felt like using a switch-statement with states for parsing my format is ugly and/or bad style.
However, if you believe that this is in fact a good solution, contrary to my assessment, I would of course also be happy with that answer.

The private static final int values in the snippet below will be replaced with an enum eventually, but for this purpose, I figured this would be more readable (by removing the need for an additional class).

private static final String SEPARATOR = "###";
private static final int SEPARATOR_EXPECTED = 0;
private static final int MATCH_EXPECTED = 1;
private static final int REPLACE_EXPECTED = 2;

public static List<MatchReplacePair> readRegexesFromFile(File file) {
    List<MatchReplacePair> list = new ArrayList<>();
    try (BufferedReader br = new BufferedReader(new FileReader(file))) {
        String line = br.readLine();
        int state = SEPARATOR_EXPECTED;
        MatchReplacePair pair = new MatchReplacePair("", "");
        while (line != null) {
            switch (state) {
            case SEPARATOR_EXPECTED:
                if (line.startsWith(SEPARATOR)) {
                    state = MATCH_EXPECTED;
                } else {
                    throw new IllegalStateException("Separator expected, but not found.");
                }
                break;
            case MATCH_EXPECTED:
                pair.setMatch(line);
                state = REPLACE_EXPECTED;
                break;
            case REPLACE_EXPECTED:
                pair.setReplace(line);
                // note: I have overridden the clone() method for this purpose, so it actually does what it's supposed to.
                list.add(pair.clone());
                state = SEPARATOR_EXPECTED;
                break;
            default:
                throw new IllegalStateException("Unknown state. Please contact the developer.");
            }
            line = br.readLine();
        }
        if (state != SEPARATOR_EXPECTED) {
            throw new IllegalStateException(
                    "Incorrect file contents: make sure the file ends with a \"replace\" block.");
        }
    } catch (IOException e) {
        e.printStackTrace(log);
    }
    return list;
}

A sample file to parse:

### //This is where one could write comments if one was so inclined.
match
replace

Although you didn't share MatchReplacePair I assume it holds the Strings that were loaded from the file. So in fact you read all the file's contents into memory.

Starting with Java 7, this can be achieved in one method call:

List<String> lines = Files.readAllLines(file.toPath());

then, you can skip the state machine implementation and just iterate over the list

if (lines.size() % 3 != 0) {
   // Incorrect file contents
}
Iterator<String> itr = lines.iterator();
// the above check ensures you can iterate over three lines safely
while (itr.hasNext()) {
  itr.next(); // ignore separator line? or you can verify if you wish
  list.add(new MatchReplacePair(itr.next(), itr.next()));
}

This also eliminates the need for clone() which was used as constructor replacement.


Mixin both instance and class methods in Ruby I have a Ruby class into which I want to include both class and instance methods. Following the pattern described in "Ruby Pattern: Extend through Include", I'm currently using the following:

class SomeObject

  include SomeObject::Ability

  def self.some_builder_method(params)
    # use some_class_method ...
  end

end

module SomeObject::Ability

  module ClassMethods

    def some_class_method(param)
      # ...
    end

  end

  def self.included(klass)
    klass.extend(ClassMethods)
  end

  def some_instance_method
    # ...
  end

end

I'd rather not make two separate modules, one being included and the other being extended, because all the methods in my module logically fit together. On the other hand, this pattern requires me to:

Define an additional ClassMethods module.
Write a boilerplate self.included method for every module.

Is this the most idiomatic way to approach this?

You're doing it right :) It's a very common idiom, and widely accepted as the preferred way to handle this.

In case you're still in doubt, here are a few examples of the same pattern found in major projects out in the wild: sidekiq, mongoid and datamapper.


Replacing strings of blanks by tabs and blanks to achieve same spacing #include <stdio.h>

#define MAXLINE 1000    /* Maximum length of a line */
#define TABSTOP 4       /* Length of tabstop */

int getLine(char line[], int limit);
int lookAhead(char line[], int start, int end);

    /*
     * Exercise 1-21
     * Write a program entab that replaces strings of blanks
     * by the minimum number of tabs and blanks to achieve the
     * same spacing.
     */

int main(void)
{
    int tabIndex, i;
    char line[MAXLINE];

    while ((getLine(line, MAXLINE)) > 0) {
        i = 0;
        while (line[i] != '\0') {
            if (line[i] == ' ') {
                tabIndex = i + (TABSTOP - (i % TABSTOP));
                if ((lookAhead(line, i + 1, tabIndex)) == 1) {
                    putchar('\t');
                    i = tabIndex;
                } else {
                    putchar(line[i]);
                    ++i;
                }
            } else {
                putchar(line[i]);
                ++i;
            }
        }
    }
    return 0;
}

int getLine(char line[], int limit)
{
    int inputVal, i;

    for (i=0; i < (limit - 1) && (inputVal = getchar()) != EOF &&
            inputVal != '\n'; ++i) {
        line[i] = inputVal;
    }
    if (inputVal == '\n') {
        line[i] = inputVal;
        ++i;
    }
    line[i] = '\0';
    return i;
}

int lookAhead(char line[], int start, int end)
{
    int i, clearPath;

    clearPath = 1;
    for (i = start; i < end; ++i) {
        if (line[i] != ' '){
            clearPath = 0;
        }
    }
    return clearPath;
}

I don't think a proper getline() function should return an int, whether it's based on the attempted read (a boolean) or something else.

I believe you're trying to imitate fgets(), which instead returns a char* (the extracted file line) if the read was successful or NULL if it failed.  A proper I/O function should set an error flag upon a failed read attempt, and this is not done by returning an int.  Plus, there will be issues if the caller of your function expects a string or a failbit, which should be the case if the caller expects proper feedback.

Unless fgets() really doesn't satisfy your needs here, I'd recommend using that or another library function that handles this properly.  Recreating an I/O function can cause problems if not written carefully, and even then you shouldn't be doing that.


Yandex programming contest: Alarms I've tried to solve this challenge about 4 hours during the contest, but all my attempts exceeded the time limit. I tried to solve it with min-heap, but can't pass all the tests. How it can be solved within given time and space requirements?

  Problem statement

  Programmer Alexey likes to work at night and does not like to come to
  work late. In order to precisely wake up in the morning, Alexey every
  evening starts \$N\$ alarm clocks on his phone. Each alarm clock is
  arranged in such a way that it rings every \$X\$ minutes from the time
  at which it was turned on.

  For example, if the alarm clock was started at the moment of time
  \$t_i\$, then it will ring at the moments of time \$t_i, t_i + X, t_i + 2 * X\$ and so on. Moreover, if some two alarms begin to ring at one time, only one of them is displayed.

  It is known that before waking up, Alexey listens every morning to
  exactly \$K\$ alarm clocks, and then wakes up. Determine the point in
  time when Alex wakes up.

  Input format

  Input format The first line contains three integers. \$N, X\$ and \$K\$ \$(1 ≤ N ≤10^5, 1≤X, K≤10^9)\$ - the number of alarms, the frequency of
  calls and the number of alarms that need to be turned off in order for
  Alex to wake up. The second line contains N integers - the points in
  time at which the alarm clocks were entered.

  Requirements

Time limit: 2 seconds
Memory limit: 256Mb

  Examples

  Example 1

  Input

6 5 10
1 2 3 4 5 6

  Output 10

  Example 2

  Input

5 7 12
5 22 17 13 8

  Output 27

  Notes

  In the second example, there are 5 alarm clocks with a frequency of 7
  calls. For example, the first alarm clock will ring at times 5, 12,
  19, 26, 33, etc. If you look at all the alarms at the same time, they
  will ring at the following times: 5, 8, 12, 13, 15, 17, 19, 20, 22
  (2nd and 5th alarm clocks at the same time), 24, 26, 27, 29,…. On the
  12th call Alexey must wake up, What corresponds to the point in time
  27.

My solution

Classified as "time limit exceeded"

import heapq

def main():
    n, x, k = map(int, input().split(" "))
    times = list(map(int, input().split(" ")))

    heapq.heapify(times)

    answers = []

    while k:
        v = heapq.heappop(times)
        heapq.heappush(times, v + x)

        if len(answers) == 0 or answers[len(answers)-1] != v:
            answers.append(v)
            k -= 1

    print(answers[k-1])

if __name__ == "__main__":
    main()

Not all counting problems require enumerating the items being counted. If you were asked to count how many sheep there are in total if there are \$n\$ trucks with \$k\$ sheep each, you could write something like:

total_sheep = 0
for truck in range(n):
    for sheep in range(k):
        total_sheep += 1

Or you could cut to the chase and compute it as:

total_sheep = n * k

The problem you are trying to solve is more subtle, but can be attacked in a similar fashion. Modular arithmetic will be your friend in doing this. Rather than looking at the setting times as single numbers, convert them to tuples of (quotient, remainder) after dividing by \$X\$. This would e.g. convert the list of times for the second example into:

 5 --> (0, 5)
22 --> (3, 1)
17 --> (2, 3)
13 --> (1, 6)
 8 --> (1, 1)

We can use this information to prune the list of alarms: if any two alarms have the same remainder, they will ring at the same time, so we only need to keep the smaller one. This would convert the above list, after also sorting it, into:

 5 --> (0, 5)
 8 --> (1, 1)
13 --> (1, 6)
17 --> (2, 3)

Those two numbers tell us in which of the \$X\$ minute periods the alarm sounds for the first time, and at what offset into that period does it first go off. So we can process it sequentially and know that:

at the end of the first (0) period, 1 alarm has sounded for the first time, a total of 1 alarm will sound in every subsequent period, and the total number of alarms sounded is also 1.
at the end of the second (1) period, 2 alarms have sounded for the first time, 3 alarms will sound in every subsequent period, and the total number of alarms sounded is 4.
at the end of the third (2) period, 1 new alarm has sounded, 4 alarms will sound every period, and a total of 8 alarms will have sounded.

Since there are no more alarms to process, we have 8 alarms so far, 4 more sounding each period, and want to reach a total of 12, some simple math tells us that this will happen during the fourth (3) period, and that it will be the last of the alarms that will reach it.

To make the math more clear, lets imagine that we wanted to reach a total of 14 alarms instead. Since 8 have already sounded, we have 14 - 8 = 6 more to go. Since 4 alarms will sound in each period, and the quotient and remainder of 6 divided by 4 are 1 and 2, we know that we will reach our target after 1 full more period, plus 2 of the four alarms in the next period. This translates to 4 full periods, plus the time for the second alarm in that period to sound. The time of 4 full periods is 7 * 4 = 28. We need to add the offset of the second alarm when sorting them by offset, not by start time, so we need to add 3, not 1, and the end result would be 31.

This general outline omits many gruesome details, like what to do if we reach the target number of alarms before we finish processing the list. And the math described above is hard to get right in all corner cases, as there are many chances for off-by-one errors. I'm not going to spoil the fun for you by coding it up: give a try, and I'll be happy to review your next version.


Parallel sieve of Eratosthenes For the first time, I tried to use threads by myself in order to implement a parallel sieve of Eratosthenes. The trick is that each time a prime is found, a thread is spawned to eliminate all the multiples of this prime number from the boolean vector (the one that tells whether a number is a prime or not). Here is my code:

#include <cmath>
#include <functional>
#include <thread>
#include <vector>

// Invalidates the multiples of the given integer
// in a given boolen vector
template<typename Integer>
void apply_prime(Integer prime, std::vector<bool>& vec)
{
    for (Integer i = prime*2u ; i < vec.size() ; i += prime)
    {
        vec[i] = false;
    }
}

template<typename Integer>
auto sieve_eratosthenes(Integer n)
    -> std::vector<Integer>
{
    std::vector<bool> is_prime(n, true);
    std::vector<std::thread> threads;
    std::vector<Integer> res;

    auto end = static_cast<Integer>(std::sqrt(n));
    for (Integer i = 2u ; i <= end ; ++i)
    {
        // When a prime is found,
        // * add it to the res vector
        // * spawn a thread to invalidate multiples
        if (is_prime[i])
        {
            res.push_back(i);
            threads.emplace_back(apply_prime<Integer>,
                                 i, std::ref(is_prime));
        }
    }

    for (auto& thr: threads)
    {
        thr.join();
    }

    // Add the remaining primes to the res vector
    for (Integer i = end+1u ; i < is_prime.size() ; ++i)
    {
        if (is_prime[i])
        {
            res.push_back(i);
        }
    }
    return res;
}

The primes are added in two steps to the res vector: every prime \$ p \$ such as \$ p < \sqrt{n} \$ is added when the prime is found, before the corresponding thread is thrown. The other primes are added at the end the of the function. Here is an example main:

int main()
{
    auto primes = sieve_eratosthenes(1000u);

    for (auto prime: primes)
    {
        std::cout << prime << " ";
    }
}

I was pretty sure that I would get some problems due to parallelism, but for some reason, it seems to work. I got the expected results in the right order. Just to be sure, I would like to know whether my program is or correct or whether it has some threading issues that I couldn't see.

Note: I used many of the ideas from the answer to improve the code and wrote a follow-up question.

As you are correctly assuming there are multiple threading related issues with your code, but lets tease you and start with the usual suspects.

Naming

The name apply_prime is misleading and inexpressive.
Neither does the function really require a prime nor does apply do it justice.
You should name it something along the lines: strike_out_multiples or something similar.

Efficiency

Your loop can be twice as fast by doing steps of two instead of one.

And now for the main act:

Thread safety

I am no expert here but I can spot at least two problems:

Ordering:

Nothing prevents the system from "favoring" your main thread and letting all others run after the main thread is hitting the join loop. This results in wrong results as the mainthread plowed through the completely true vector before anyone could tell it that for example 4 is not prime.

Parallel writes:

This might be a bit of a corner case and not a problem anymore but at least in the old standard std::vector<bool> was a specialization that used only one bit per value. While this saves you some space it comes with the cost of actually accessing 8 bits when only working on one. This means that two threads might well be working on different bits but in the same byte. Consider this:

Thread 1 writes false to bit 4 while thread 2 writes false to bit 6. Both are located in byte 0. Now both start out with reading the initial value of the byte, say (true, true, true, false, true, true, true, true) and stores them! Now one of the two threads will write its value later than the other and overwrites the false of the other one -> lost update.

Maybe this can even happen with byte sized booleans as some architectures do not allow for only byte-wise access but only word wise (which results in the same problem, only with wider sizes).


Merge sort for a List Is this merge sort following good coding standards? What are some improvements I can make to make it cleaner?

public final class MergeSort {

    private MergeSort() {

        // prevent instantiation

    }

    private static Object[] temp;

    public static <T extends Comparable<? super T>> void sort(List<T> array){

        Object[] a = array.toArray();

        temp = new Object[a.length];

        sort(a, 0, a.length - 1);

        ListIterator<T> i = array.listIterator();

        for (int j=0; j<a.length; j++) {

            i.next();

            i.set((T)a[j]);

        }

    }

    private static void sort(Object[] a, int lo, int hi) {

        if(hi <= lo){

            return;

        }

        int mid = lo + (hi - lo)/2;

        sort(a, lo, mid);// sort left part

        sort(a, mid+1, hi); // sort right part

        merge(a, lo, mid, hi);

    }

    private static void merge(Object[] a, int lo, int mid, int hi) {

        int i = lo, j = mid+1;

        System.arraycopy(a, lo, temp, lo, hi + 1 - lo);

        for(int k = lo;k <= hi; k++){

            if (i > mid) a[k] = temp[j++];

            else if (j > hi ) a[k] = temp[i++];

            else if (((Comparable)temp[j]).compareTo(temp[i]) <= -1) a[k] = temp[j++];

            else a[k] = temp[i++];

        }

    }

}

Binary operators should always be surrounded by whitespaces(it is inconsistent in your code: sometimes you do surround them, sometimes you don't). For example,

for (int j=0; j<a.length; j++)

should be

for (int j = 0; j < a.length; j++)

There should be one whitespace before an opening bracket. Again, there is an inconsistency in your code: a whitspace is present most of the time, but it is not present, for instance, here:  if(hi <= lo){. Not following established style guidelines is not good, having inconsistencies is definitely bad.
A little bit about blank lines: they should be present between different methods and it is fine to have them inside one method if it is really big and you need to separate logical blocks from each other(however, too long methods should be avoided). But having a blank line after each statement is pointless and it makes your code less readable.

private static void sort(Object[] a, int lo, int hi) {
    if(hi <= lo) {
        return;
    }
    int mid = lo + (hi - lo) / 2;
    sort(a, lo, mid); // sort left part
    sort(a, mid + 1, hi); // sort right part
    merge(a, lo, mid, hi);
}

looks better than:

private static void sort(Object[] a, int lo, int hi) {

    if(hi <= lo){

        return;

    }

    int mid = lo + (hi - lo)/2;

    sort(a, lo, mid);// sort left part

    sort(a, mid+1, hi); // sort right part

    merge(a, lo, mid, hi);

}

Variable naming. There is no need to shorten words: you code will become more readable if you use names like low and high instead of lo and hi.
It is conventional to declare one variable at a time. I would use

int i = lo;
int j = mid + 1;

instead of int i = lo, j = mid+1;. And again, i and j are not the best names for variables because they are not descriptive(but it can be ok if their scope is very narrow, so it is not a very big issue in this case).
It is a good practice to write comments for each public method to describe what it does, what do its parameters stand for, what pre- and post-conditions are guaranteed, what exceptions can be thrown, is it thread-safe or not and so in. You should write comments for all public classes, too.


Reduce size of multiple uploaded images I am a young programmer with the ability to learn more. I created a public site where a user can upload multiple pictures at once. The code works successfully and inserts the images into a MySQL database. As I improved the code further by reducing the size of multiple uploaded pictures at once, only one picture was uploaded into the folder and MySQL. Could anyone please help me on this? Is there any style or code to reduce the size of multiple images uploaded by the user?

DS in my code is DIRECTORY_SEPARATOR define in initialize file.

if(isset($_POST['submitImage']))
{
    $file = $_FILES['upload'];
    $errors = array();

    if(empty($file))
    {
        $errors[] = ' The file could not be empty ';
    }
    else
    {
        foreach( $file['name'] as $key => $value )
        {
            $extensions = array("jpeg","jpg","png");
            $file_ext=explode('.',$file['name'][$key]);
            $file_ext=end($file_ext);
            //$file_ext=strtolower(end(explode('.',$file['name'][$key])));

            if(in_array($file_ext,$extensions ) === false)
            {
                $errors[] = "extension not allowed";
            }
            else
            {
                $filetmp = $file['tmp_name'][$key];
                $terget_path = SITE_ROOT.'j2reimage'.DS.'image'.DS;
                $dat = strftime("%Y-%m-%d %H:%M:%S", time());

                if(file_exists($terget_path))
                {
                    //move_uploaded_file( $filetmp, $terget_path.'/'.$file['name'][$key]);
                    mysql_query("insert into 4j2memberimage values ('', $memmem->id,'{$file['name'][$key]}', '$dat')");

                    if(move_uploaded_file($filetmp, $terget_path.'/'.$file['name'][$key] ))
                    {
                        $exe = explode(".", $file['name'][$key]);
                        $ext = $exe[1];
                        $w = 250;
                        $h = 250;
                        $target  = $terget_path.DS.$file['name'][$key];
                        $newcopy = SITE_ROOT.'j2reimage'.DS.'reCopy'.DS.$file['name'][$key];
                        list($w_orig, $h_orig) = getimagesize($target);
                        $scale_ratio = $w_orig / $h_orig;

                        if (($w / $h) > $scale_ratio)
                        {
                            $w = $h * $scale_ratio;
                        }
                        else
                        {
                            $h = $w / $scale_ratio;
                        }

                        $img = "";
                        $ext = strtolower($ext);

                        if ($ext == "gif")
                        {
                            $img = imagecreatefromgif($target);
                        }
                        else if($ext =="png")
                        {
                            $img = imagecreatefrompng($target);
                        }
                        else
                        {
                            $img = imagecreatefromjpeg($target);
                        }

                        $tci = imagecreatetruecolor($w, $h);
                        imagecopyresampled($tci, $img, 0, 0, 0, 0, $w, $h, $w_orig, $h_orig);

                        if(@imagejpeg($tci, $newcopy, 80))
                        {
                            //return true;
                            $terger_path = $target;
                            //$terger_path = SITE_ROOT.DS.$this->img_path.DS.$this->filename;
                            return unlink($terger_path) ? true : false;
                        }
                    }
                    //end of if file_exists
                }
                else
                {
                    $errors[] = 'Upload extention could not be located';
                }
            }
            //redirect_to('inb.php');
        }
    }
}

In your HTML View you can use this
<file name="upload[]" accept="image/*" multiple> multiple and accept attribute is only available in HTML5 dosen't work in old browsers.

<?php
    $files = null;
    $errors = null;

    define('IMAGE_TARGET',SITE_ROOT.'j2reimage'.DS.'image');

    if(!empty($_POST['submitImage']))
    {
        $files = $_FILES['upload'];
        $errors = array();

        if(!empty($files))
        {
            //Move extension list to here you don't need redeclarate variables inside loop
            $valid_extensions = array('jpeg','jpg','png','gif');

            foreach($files as $file)
            {

                    //Don't use explode for extensions
                    //$file_ext = explode('.',$file['name'][$key]);
                    //Use pathinfo
                    $path_parts = pathinfo($file);
                    $file_ext = $path_parts['extension'];

                    //$file_ext=strtolower(end(explode('.',$file['name'][$key])));

                    if(in_array($file_ext,$valid_extensions) === false)
                    {
                        $errors[] = ".$file_ext extension not allowed";
                    }
                    else
                    {
                        $filetmp = $file['tmp_name'][$key];

                        $dat = strftime("%Y-%m-%d %H:%M:%S", time());

                        if(file_exists(IMAGE_TARGET.DS.$file['name']))
                        {
                            //move_uploaded_file( $filetmp, $terget_path.'/'.$file['name'][$key]);
                            mysql_query("INSERT INTO 4j2memberimage VALUES ('', $memmem->id,'{$file['name']}', '$dat')");

                            //if(move_uploaded_file($filetmp, $terget_path.'/'.$file['name'][$key] ))

                            //Use copy insteds of  move_uploaded_file sometimes you have right problems in some servers.

                            if(copy($filetmp, IMAGE_TARGET.DS.$file['name'] ))
                            {
                                $exe = explode(".", $file['name']);
                                $ext = $exe[1];
                                $w = 250;
                                $h = 250;
                                $target  = IMAGE_TARGET.DS.$file['name'];
                                $newcopy = IMAGE_TARGET.DS.'thumb_'.DS.$file['name'];

                                list($w_orig, $h_orig) = getimagesize($target);
                                $scale_ratio = $w_orig / $h_orig;

                                if (($w / $h) > $scale_ratio)
                                {
                                    $w = $h * $scale_ratio;
                                }
                                else
                                {
                                    $h = $w / $scale_ratio;
                                }

                                $img = '';
                                $ext = strtolower($ext);

                                switch($ext) {
                                    case 'gif':
                                        $img = imagecreatefromgif($target);
                                        break;
                                    case 'png':
                                        $img = imagecreatefrompng($target);
                                        break;
                                    case 'jpeg':
                                    case 'jpg':
                                        $img = imagecreatefromjpeg($target);
                                        break;
                                }

                                $tci = imagecreatetruecolor($w, $h);
                                imagecopyresampled($tci, $img, 0, 0, 0, 0, $w, $h, $w_orig, $h_orig);

                                if(@imagejpeg($tci, $newcopy, 80))
                                {
                                    //return true;
                                    $target_path = $target;
                                    //$terger_path = SITE_ROOT.DS.$this->img_path.DS.$this->filename;
                                    file_put_contents($target_path,$img);
                                    //return unlink($target_path) ? true : false;
                                }
                            }
                            //end of if file_exists
                        }
                        else
                        {
                            $errors[] = 'Upload extention could not be located';
                        }
                    }
                    //redirect_to('inb.php');
                }//foreach
        }
        else
        {
            $errors[] = ' The file could not be empty ';
        }//if
    }//if


Combining horizontal and vertical sliders into one I'm combining 2 UISliders made from scratch -- one horizontal and one vertical -- to make a single slider that controls 2 parameters. They are set up so that the  vertical slider is perpendicular to the horizontal one. (Like a cross) The thumb knob can move both horizontally and vertically, and if it's moved horizontally, the vertical tracker will move left and right to follow the thumb.

I'm having trouble setting the boundaries of the thumb. Its leftmost limit is supposed to be reached when the left edge of the v. slider touches the left edge of the horizontal slider, and the rightmost limit is when the right edges of the sliders intersect. For the sake of consistency, I took whatever code I needed to obtain the previous conditions and used a similar structure for the top and bottom limits -- even though there is no actual intersection.

This is what I came up with. (This is in a custom UIView which holds both sliders. Also, the size of the UIView is 100 x 100, and the sliders are each 90 points long.)

- (BOOL)continueTrackingWithTouch:(UITouch *)touch withEvent:(UIEvent *)event {

CGPoint touchPoint = [touch locationInView:self];

CGFloat hLimitLeft = horizontalSliderFrame.origin.x + .5* ksliderThickness;
CGFloat hLimitRight = horizontalSliderFrame.origin.x + horizontalSliderFrame.size.width - .5* ksliderThickness;
CGFloat vLimitTop = verticalSliderFrame.origin.y + .5* ksliderThickness;
CGFloat vLimitBottom = verticalSliderFrame.origin.y +  verticalSliderFrame.size.height - .5* ksliderThickness;

CGRect thumbBoundaries = CGRectMake(hLimitLeft, vLimitTop, hLimitRight - hLimitLeft, vLimitBottom - vLimitTop) ;

if (CGRectContainsPoint(thumbBoundaries, touchPoint)) {

    self.thumb.center = CGPointMake(touchPoint.x, touchPoint.y);

    [self setNeedsDisplay];
    [self updateValues];

}

return YES;

}

Generally, it works fine. However, it doesn't move at the edges because the touch usually lies outside the boundaries even when it's holding on to the thumb knob.

I'm wondering if there's a better way to design this code.

Once your control is active, just keep tracking until the touch ends, even if the touch moves outside of the view:

- (BOOL)continueTrackingWithTouch:(UITouch *)touch withEvent:(UIEvent *)event {
    CGPoint touchPoint = [touch locationInView:self];

    CGFloat hLimitLeft = horizontalSliderFrame.origin.x + .5* ksliderThickness;
    CGFloat hLimitRight = hLimitLeft + horizontalSliderFrame.size.width - ksliderThickness;
    CGFloat vLimitTop = verticalSliderFrame.origin.y + .5* ksliderThickness;
    CGFloat vLimitBottom = vLimitTop +  verticalSliderFrame.size.height - ksliderThickness;

    self.thumb.center.x = MAX(hLimitLeft, MIN(hLimitRight, touchPoint.x));
    self.thumb.center.y = MAX(vLimitTop, MIN(vLimitBottom, touchPoint.y));

    [self setNeedsDisplay];
    [self updateValues];

    return YES;
}

Note also that I changed the calculation of hLimitLeft and vLimitTop. I think this approach is clearer (in particular I missed the different signs on the .5 * ksliderThickness term the first couple of times through).

It's been a while since I've done iOS development, so there might be some minor mistakes, but this should get you going in the right direction.


Filtering an array according to 7 checkboxes in a form I am filtering an array whenever checkboxes are checked.

if (this.deliveryConcession[0].checked) {
            this.allItems = this.allItems.filter(fil => fil.deliveryconcession.readytoship === this.deliveryConcession[0].checked);
        }
        if (this.deliveryConcession[1].checked) {
            this.allItems = this.allItems.filter(fil => fil.deliveryconcession.instantdownload === this.deliveryConcession[1].checked);
        }
        if (this.deliveryConcession[2].checked) {
            this.allItems = this.allItems.filter(fil => fil.deliveryconcession.unespecifiedshipment === this.deliveryConcession[2].checked);
        }
        if (this.seatConcession[0].checked) {
            this.allItems = this.allItems.filter(fil => fil.seatingConcession.parking === this.seatConcession[0].checked);
        }
        if (this.seatConcession[1].checked) {
            this.allItems = this.allItems.filter(fil => fil.seatingConcession.restrictedview === this.seatConcession[1].checked);
        }
        if (this.seatConcession[2].checked) {
            this.allItems = this.allItems.filter(fil => fil.seatingConcession.wheelchair === this.seatConcession[2].checked);
        }
        if (this.seatConcession[3].checked) {
            this.allItems = this.allItems.filter(fil => fil.seatingConcession.alcoholFree === this.seatConcession[3].checked);
        }

There are 7 checkboxes, each associated with an object.

  seatConcession = [
        { id: 1, name: 'Parking pass included', checked: false },
        { id: 2, name: 'Unrestricted view', checked: false },
        { id: 3, name: 'Wheel chair accessible', checked: false },
        { id: 4, name: 'Without age restrictions', checked: false }
    ];
    deliveryConcession = [
        { id: 1, name: 'Ready to ship(paper)', checked: false },
        { id: 2, name: 'Instant download(e-ticket)', checked: false },
        { id: 3, name: 'Unspecified shipment(paper)', checked: false }
    ];

How can i improve the above with simple loadash filter or another way?

The first thing I notice is that this:

if (this.deliveryConcession[0].checked) {
  this.allItems = this.allItems.filter(fil => fil.deliveryconcession.readytoship === this.deliveryConcession[0].checked);
}

Is the same as

if (this.deliveryConcession[0].checked) {
  this.allItems = this.allItems.filter(fil => fil.deliveryconcession.readytoship);
}

As a second step I would refactor this to something like:

deliveryConcession = [
    { id: 1, name: 'Ready to ship(paper)', checked: false, key: 'readytoship' },
   ...
];

function filterItems(items, concessions, name) {
  for (let k in concessions) {
    let concession = concessions[k];
    if (concession.checked)
      items = items.filter(fil =>  fil[name][concession.key] )
  }
  return items;
}

this.allItems = filterItems(this.all Items, this.deliveryConcessions, 'deliveryConcession');

An alternative might be to define your hash as:

deliveryConcession = [
    { id: 1, name: 'Ready to ship(paper)', checked: false, filter: fil => fil.deliveryConcession.readyToShip },
   ...
];

function filterItems(items, concessions, name) {
  for (let k in concessions) {
    let concession = concessions[k];
    if (concession.checked)
      items = items.filter(concession.filter)
  }
  return items;
}


Program to Check If string is Palindrome (in C) Just wrote a program to check if a string is Palindrome. If that's true, return 1. Else, return 0. How can I improve it?

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

int palindrome(char *s)
{
    int i = 0, length = strlen(s) - 1, n = length;
    int boolean = 0;

    for (; i <= length && n >= 0; i++, n--) {
        if (toupper(s[i]) == toupper(s[n]))
            boolean = 1;
        else
            boolean = 0;
    }

    return boolean;
}

int main(void)
{
    char string[] = "Racecar";

    printf("%d", palindrome(string));

    return 0;
}

Bug
Given the input "racebar", the function returns true.  The problem is that your boolean variable is reset after each character pair, so you are only really testing the first character with the last.  I would do the following:

Get rid of the boolean variable and return false on the first mismatch.
Fix the loop iteration to only iterate through half the string instead of the whole string.
Mark the input parameter as const, since you do not modify the input string.  This allows your function to accept both const strings and non-const string as arguments.
In C99, you can declare loop variables inside the loop itself, which helps to reduce the scope of these variables to only where they are used.

Here is a rewrite showing all of the above:
int palindrome(const char *s)
{
    for (int i = 0, n = strlen(s) - 1; i < n; i++, n--) {
        if (toupper(s[i]) != toupper(s[n]))
            return 0;
    }

    return 1;
}


LRU Cache in constant time I would like to ask for code review for my LRU Cache implementation from leetcode.

  Design and implement a data structure for Least Recently Used (LRU) cache. It should support the following operations: get and put.

  get(key) - Get the value (will always be positive) of the key if the key exists in the cache, otherwise return -1.
  put(key, value) - Set or insert the value if the key is not already present. When the cache reached its capacity, it should
  invalidate the least recently used item before inserting a new item.

  Follow up:
  Could you do both operations in O(1) time complexity?

I make sure that I used the LinkedListNode class to make sure the solution is optimal with O(1) time complexity.

class ListNode(object):
    def __init__(self, key, val):
        self.val = val
        self.key = key
        self.next = None
        self.prev = None

class LinkedList(object):
    def __init__(self):
        self.head = None
        self.tail = None

    def insert(self, node):
        node.next, node.prev = None, None  # avoid dirty node
        if self.head is None:
            self.head = node
        else:
            self.tail.next = node
            node.prev = self.tail
        self.tail = node

    def delete(self, node):
        if node.prev:
            node.prev.next = node.next
        else:
            self.head = node.next
        if node.next:
            node.next.prev = node.prev
        else:
            self.tail = node.prev
        node.next, node.prev = None, None  # make node clean

class LRUCache(object):

    # @param capacity, an integer
    def __init__(self, capacity):
        self.list = LinkedList()
        self.dict = {}
        self.capacity = capacity

    def _insert(self, key, val):
        node = ListNode(key, val)
        self.list.insert(node)
        self.dict[key] = node

    # @return an integer
    def get(self, key):
        if key in self.dict:
            val = self.dict[key].val
            self.list.delete(self.dict[key])
            self._insert(key, val)
            return val
        return -1

    # @param key, an integer
    # @param value, an integer
    # @return nothing
    def put(self, key, val):
        if key in self.dict:
            self.list.delete(self.dict[key])
        elif len(self.dict) == self.capacity:
            del self.dict[self.list.head.key]
            self.list.delete(self.list.head)
        self._insert(key, val)

This passes the leetcode tests.

Usual comment: add """docstrings""" to public methods at the very least.

Avoid double (and triple) lookups.  Python has fast exception handling.  Instead of testing for existence, and then fetching the value, just fetch the value.  And don’t fetch it more than once.  Ie, replace this:

if key in self.dict:
    val = self.dict[key].val
    self.list.delete(self.dict[key])
    self._insert(key, val)
    return val
else:
    return -1

with this:

try:
    node = self.dict[key]
    val = node.val
    self.list.delete(node)
    self._insert(key, val)
    return val
except KeyError:
    return -1

Above, you are deleting/discarding a ListNode containing a key-value pair, and then immediately creating a new ListNode with the same key-value pair to insert.  LinkedList.insert() takes the trouble to clean the node for insertion; why not reuse the node?  You wouldn’t have to update self.dict[key], either.

Similar optimizations for put():  don’t do double lookups and possibly reuse node (but with updated .val).


Picking a random neighbouring cell, but only picking previously-visited ones if there aren't any unvisited I'm creating mazes, and have a Cell class (only relevant code shown)...

public class Cell {
  public int Row { get; }
  public int Col { get; }

  public List<Cell> Links { get; }

  public Cell North { get; set; }
  public Cell South { get; set; }
  public Cell East { get; set; }
  public Cell West { get; set; }

  public Cell(int row, int col) {
    Row = row;
    Col = col;
    Links = new List<Cell>();
  }

  public bool Linked(Cell cell) =>
    Links.Contains(cell);

  public bool DeadEnd =>
    Links.Count == 1;

  public IEnumerable<Cell> Neighbours =>
    new List<Cell> { North, South, East, West }.Where(c => c != null);

}

The mazes are all (currently) rectangular, so a Cell starts off with 2, 3 or 4 neighbours (depending on where it is in the maze, eg corner cells only have two neighbours, other edge cells have three, internal cells have four). These are set during the maze initialisation code (In Maze class which is not shown as it's not relevant here), and the Neighbours property gives the resulting neighbouring cells.

Given a grid of cells, the maze-generation algorithm links cells together using Link() and Unlink() methods (not shown as they aren't relevant here).

The Linked method then allows you to check if a cell is linked to the current one.

As I often need to pick a random cell from a collection, I have the following extension method defined...

public static Cell Rand<Cell>(this IEnumerable<Cell> items, Random r) =>
  items.OrderBy(n => r.Next()).First();

I create one Random object at the start, and pass it around, hence the second parameter.

Some of the maze-generation algorithms work by taking a random walk around the grid. When on a cell, they pick a neighbouring cell to be the next in the walk.

One specific algorithm will try and avoid previously visited cells. This means that if there are any neighbours that don't yet have any links, then it will pick one of those. If all neighbouring cells have links, it picks one of those instead.

My current code for this is shown below. This is on the maze-generating class, and current is the cell we're currently visiting in our random walk. We pick the next by...

Cell next = current.Neighbours.Any(c => c.Links.Count == 0)
  ? current.Neighbours.Where(c => c.Links.Count == 0).Rand(r)
  : current.Neighbours.Rand(r);

This works fine, but I can't help feeling that there is a way to combine this into one Linq query instead of the three required for the ?: operator.

Any suggestions? Thanks.

Using straight linq you could get to two iterations over Neighbours, assuming Neighbours is a class and not a struct.

var neighbour = current.Neighbours.FirstOrDefault(n => n.DeadEnd) ?? current.Neighbours.Where(n => !current.Linked(n)).First();

Pretty similar to yours except we using FirstOrDefault to return null if not found and using the ?? to execute the 2nd iteration if it's null.

If it is a struct then you would need to compare the FirstOrDefault to default.  At that point I would just do the FirstOrDefault and write an If statement on next line comparing it to default.

If you want to do it in one iteration then will need to break out into a foreach loop. The downside is it will be calling the Linked() method even if it will never be used. As needs to store the value just in case it can't find DeadEnd.

Neighbour defaultValue == null;
foreach (var neighbour in current.Neighbours)
{
    if (neighbour.DeadEnd)
    {
        return neighbour;
    } else if (defaultValue == null && !current.Linked(neighbour))
    {
        defaultValue = neighbour;
    }
}

return defaultValue;

Without knowing what Linked does - aka does it have side effects or performance issues.  This might not be the best answer.

As you can see from my review there are questions: Neighbours a class or struct?  Should post the source code for Neighbours.  What does Linked method do?  Because of this the code can't fully be reviewed and is not complete.  I would suggest you update your question to get better answers.


Writing to rotating log file via channels I'm an experienced programmer (mostly C#, JavaScript, PHP). Go is quite a paradigm shift and I want to make sure I'm starting on the right foot.

This is my first Golang package. It's purpose is to be initiated and sent log lines (in the form of map[string]string to AddEntry. It's using channels since the lines could be added VERY frequently and I don't want to consume I/O time on the receiving function (outside of this package).

Any ways this could better follow Go's practices I am definitely interested to hear.

Usage Example:

testLog := chatlog.NewChatLog("/opt/test/logs/", "IRC", "SomeServerName", 1000)
testEntry := make(map[string]string)
testEntry["Type"] = "PRIVMSG"
testEntry["Entity"] = "SomeUser"
testEntry["Content"] = "This is the message!"
testLog.AddEntry(testEntry)

The Package:

// Package chatlog provides a structure way of storing chatlogs for consumption by CelestialStats.
package chatlog

import (
    "encoding/json"
    "fmt"
    "log"
    "os"
    "path"
    "strconv"
    "time"
)

type ChatLog struct {
    Protocol   string
    Server     string
    logDir     string
    logFile    *os.File
    logChannel chan map[string]string
}

// NewChatLog returns a new ChatLog ready to recieve log entries and write them to disk.
func NewChatLog(LogDir, Protocol, Server string, MaxQueue int) *ChatLog {
    cl := new(ChatLog)
    cl.logDir = LogDir
    cl.Protocol = Protocol
    cl.Server = Server
    cl.logChannel = make(chan map[string]string, MaxQueue)
    go cl.Write()
    return cl
}

// Open opens a specific structured file for later writing.
func (chatLog *ChatLog) Open() {
    var logFilename = chatLog.GenerateFilename()
    var parentDir = path.Dir(logFilename)
    if _, err := os.Stat(parentDir); os.IsNotExist(err) {
        os.MkdirAll(parentDir, 0755)
    }
    f, err := os.OpenFile(logFilename, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644)
    if err != nil {
        log.Println("Error opening file:", logFilename)
    }
    chatLog.logFile = f
}

// AddEntry adds a map representing the chat message to the log channel. It
// also appends the current Unix Timestamp in milliseconds to the map.
func (chatLog *ChatLog) AddEntry(newEntry map[string]string) {
    newEntry["Timestamp"] = strconv.FormatInt(time.Now().UTC().UnixNano()/int64(time.Millisecond), 36)
    chatLog.logChannel <- newEntry
}

// Write outputs any additions to the log channel to the current log file.
// If the log does not exist or is old this triggers the log to be opened
// or rotated. All entries are converted to JSON and stored on object per.
// line.
func (chatLog *ChatLog) Write() {
    for i := range chatLog.logChannel {
        chatLog.RotateIfNeeded()
        jsonVal, _ := json.Marshal(i)
        _, err := chatLog.logFile.WriteString(string(jsonVal) + "\n")
        if err != nil {
            log.Println("Error writing to file:", err)
        }
        chatLog.logFile.Sync()
    }
}

// RotateIfNeeded checks if the current ChatLog struct is referencing the
// proper log file. If the reference is incorrect or doesn't exist then this
// function opens the proper log, and if necessary closes the old one.
func (chatLog *ChatLog) RotateIfNeeded() {
    if chatLog.logFile == nil {
        // Open file if not opened
        chatLog.Open()
    } else if chatLog.logFile.Name() != chatLog.GenerateFilename() {
        // If the filename doesn't match where we should be writing
        // close the old file and reopen with a new name.
        chatLog.logFile.Close()
        chatLog.Open()
    }
}

// GenerateFilename returns a filename in the following format
// using the current timestamp:
// $LOGDIR/$PROTOCOL/$SERVER/YYYY/MM/DD/HH.csl
func (chatLog *ChatLog) GenerateFilename() string {
    return path.Join(
        chatLog.logDir,
        chatLog.Protocol,
        chatLog.Server,
        time.Now().UTC().Format("2006/01/02/15.csl"))
}

First up, as I understand it, you want to write to log files, but avoid constant IO operations. That's understandable. What I struggle to understand, however, is why you wouldn't use the standard golang packages to do buffered writes? the bufio package has a buffered writer that does just that.

Seeing as this is a code review site, allow me to first address some aspects of your code that could do with some more work. A quick overview of these things:

Avoid using new(). It's more idiomatic, and flexible, to actually create a literal: new(ChatLog) => &ChatLog{}. This has the advantage of offering the visual reminder that it's a pointer (&), and allows you to initialze any exported fields.

This allows you to write the following:

cl := new(ChatLog)
cl.logDir = LogDir
cl.Protocol = Protocol
cl.Server = Server
cl.logChannel = make(chan map[string]string, MaxQueue)

As a oneliner:

cl := &ChatLog{
    logDir: logDir,
    Protocol: Protocol,
    Server: Server,
    logChannel: make(chan map[string]string, MaxQueue),
}

That, to my eye looks a lot cleaner.
A similar thing can be said about your example usage snippet. make is a construct you use to create buffered channels, slices with a pre-set length and/or capacity. If you don't need to do that, you might just as well use a literal:

testEntry := make(map[string]string)
testEntry["Type"] = "PRIVMSG"
testEntry["Entity"] = "SomeUser"
testEntry["Content"] = "This is the message!"
testLog.AddEntry(testEntry)

Can be written as:

testLog.AddEntry(
    map[string]string{
        "Type": "PRIVMSG",
        "Entity": "SomeUser",
        "Content": "This is the message!",
    },
)

A lot cleaner, I'd say.

"Errors are values, just like any other" is an adage you might have heard. This is true, but when you're doing something quite fundamental like logging, it's a really, really bad idea to ignore errors. If your logging component fails, I'd argue the best thing to do at that point is simply to call it a day, and exit. It's a fatal error IMO.

This means, for example that in your Open function:

if err != nil {
    log.Println("Error opening file:", logFilename)
}
chatLog.logFile = f

You're using the standard log package to simply print out a line saying you couldn't open the log file, and then you happily go and assign what can only be a nil pointer to chatLog.logFile. The first time you try and log something, you'll get a runtime panic because you'll attempt to dereference a nil-pointer. What I'd do at that point is log the actual error value I received and call os.Exit(1) or something:

f, err := os.OpenFile(logFilename, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644)
if err != nil {
    log.Println("Failed to open logfile %v - %v", logFilename, err)
    os.Exit(1)
}
chatLog.logFile = f

Some other things to consider (quite urgently)

Your code is not thread-safe. You're exporting both RotateIfNeeded and Write. Neither of these functions actually lock. I can see why, but what could happen if, when someone calls the RotateIfNeeded function, which then decides to open a new log, and in that time your Write function reaches the chatLog.logFile.Sync call? You close the logFile before you open a new one. These operations aren't atomic, not by a long shot, and you've got no means of locking. This is a very risky thing to do.
For every message you read from the channel, you're currently comparing the file name to a on-the-spot generated name. Wouldn't it be so much easier to move the occasional checking of the file to a routine instead? Just have it generate a new name every Nms, compare and if needed, acquire a lock on the os.File pointer, rotate the log file, and release? Whilst doing that, you'd only need to lock and release the a mutex every time you actually write a message to the log, instead of comparing file names. This has the added benefit that, owing to using a mutex, resolves to a large extent the concurrency problems mentioned earlier.
The risk of using channels for logging: You're using channels to write to a log, instead of writing to a log directly. This clearly suggests the use of routines. Given this, you have to understand that you stand to loose a great deal of logs should your main func return before you've worked through the entire set of messages on the channel. What would happen in that case? You're losing information you were trying to log. Plain and simple. This is unacceptable.
Reinventing the wheel: Logging is something that can be implemented cheaply, and without requiring the use of a mutex, or essentially re-implementing a mature tool like logrotate. Why not write your logs to stdout or stderr, and pipe those streams to a log, that is managed by logrotate for you? KISS


Calculate the time it takes to achieve your money goal in Kotlin I am writing a very simple test application in Kotlin for Android, where I just want to calculate the time it takes for someone to reach a specific amount of money, taking into count his salary and increase in salary each year.

I am very new to Android Studio, which is why I don't really know about the best practises yet. Currently, I wonder wheather I should put the EditText into global scope, so that I can access them without passing any parameters to all the functions.

This is my class:

class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        findViewById(R.id.resultsButton).setOnClickListener {

            val moneyField: EditText = findViewById(R.id.moneyField) as EditText
            val moneyGoal: Float = getNumberFromTextField(moneyField)

            val salaryField: EditText = findViewById(R.id.salaryField) as EditText
            val salary: Float = getNumberFromTextField(salaryField)

            val increaseField: EditText = findViewById(R.id.increaseField) as EditText
            val increase: Float = getNumberFromTextField(increaseField)

            calculateAndDisplayTimeUntilMoneyGoal(moneyGoal, salary, increase)
        }
    }

    fun getNumberFromTextField(field: EditText): Float {
        return field.text.toString().toFloat()
    }

    fun calculateAndDisplayTimeUntilMoneyGoal(goal: Float, salary: Float, increaseInPercent: Float){
        var moneyEarned: Float = 0f
        var currentSalary = salary
        var months: Int = 0
        var yearsPassed: Int = 0

        while (moneyEarned < goal){
            moneyEarned += currentSalary
            months++

            if (oneYearPassed(months)){
                currentSalary += increaseSalary(currentSalary, increaseInPercent)
                months = 0
                yearsPassed++
            }
        }

        val resultsString = "After $yearsPassed years and $months months you will have earned $moneyEarned!"
        (findViewById(R.id.displayText) as TextView).text = resultsString
    }

    fun oneYearPassed(months: Int): Boolean = (months == 12)

    fun increaseSalary(currentSalary: Float, increaseInPercent: Float): Float {
        return (currentSalary / 100) * increaseInPercent
    }
}

Any suggestions on how I can improve it? (Altough Kotlin is new to me, I have experience in other programming languages)

Hi yes I would store the textEdit references in a field of the class (not actual global scope), if you want to access them more often.

Running through the view hierarchy to find it on each click is not the most performant thing for more complex layout and so not best practice. These references stay valid until onDestroyed is called, so it's ok to store them.

BUT with Kotlin you would usually do it like this without casts etc. https://kotlinlang.org/docs/tutorials/android-plugin.html

import kotlinx.android.synthetic.main.activity_main.*

class MyActivity : Activity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        resultsButton.setOnClickListener {
            // Either try catch NumberFormatException or validating the string with
            // TextWatcher will be required to avoid problems here, but also in the original code.
            calculateAndDisplayTimeUntilMoneyGoal(
                goal: Float.valueOf(moneyField.getText().toString()),
                salary: Float.valueOf(salaryField.getText().toString()),
                increaseInPercent: Float.valueOf(increaseField.getText().toString()))
        }
    }
};