#Hi! Is my code any good? ><

1 messages · Page 1 of 1 (latest)

dire wyvern
brisk igloo
#

Looks pretty good! I think there are a few ways you can improve though:

  • You generally wouldn't put your JSON files in src/. You could either put it in the root of the repository or create a new folder
  • Your usage text would probably be easier to read in the source code if you used a multiline string rather than \n for newlines
  • When you pipe into a function that's not the first argument, you should use function captures rather than anonymous functions (e.g. 2 |> add(1, _) not 2 |> fn(x) { add(1, x) })
  • I think you're overusing pipes into anonymous functions in general, for example here, I think it would be clearer to just declare some variables rather than piping into anonymous functions
  • If you want to prepend to a list, use list.prepend(list, value), not list.append([value], list)
dire wyvern
#

Woah! Function captures are a game changer! Thank you!

topaz ore
#

something that I noticed:

  let data = case simplifile.read(filepath) {
    Ok(data) -> data
    Error(_) -> "no file"
  }

  case json.parse(data, parse_done) {
    Ok(done) -> Ok(done)
    Error(_) -> Error(Nil)
  }

When read returns Error(_), you set data as "no file", making json.parse handle something that is not intended to be parsed into json. I think it would be better to do something like:

case simplifile.read(filepath) {
    Ok(data) ->
      json.parse(data, parse_done)
      |> result.replace_error(Nil)
    Error(_) -> Error(Nil)
  }
}
dire wyvern
#

Oh, I somehow skipped pretend while reading documentation

dire wyvern
topaz ore
#

oh and something that I would do, load_done & load_tasks do pretty much the same thing, taking file content and parsing it with decoder. The only difference between them is decoder itself. You can make a function that will accept decoder as an argument, that way you dont repeat the parsing code twice

#

as for stuff that you plan to implement (ex TODO reset), you can use todo keyword: todo as "implement reset". That way language server and compiler will warn you about todos. This helps me not forget about them, and its easy to track what I have not done yet

dire wyvern
topaz ore
#

nice! If you dont mind, here is also what I see that can be improved:

in reset:

      let res = case what {
        Tasks -> overwrite([], done)
        Done -> overwrite(tasks, [])
        All -> overwrite([], [])
      }
      case res {
        Ok(_) -> Ok(Nil)
        Error(_) -> Error(Nil)
      }

since overwrite returns Result(Nil, simplifile.FileError), you can pipe result of first case into result.replace_error, since it does the same thing.

And in load, I see:

  let field = case what {
    Tasks -> "tasks"
    Done -> "done"
    All -> "won't happen"
  }

I think there could be better ways of implementing load function. With current approach you need to remember to not pass All as an argument. With small & simple codebase it could be fine, but with something more complicated that should not be a thing. There is few options here:

  1. Bring back load_done & load_tasks, and make them create their own decoders and call parse_file function that will take these decoders and do the parsing...
  2. Or make sure that All is being handled the right way, though this will require more refactor or maybe change in your app logic
dire wyvern
topaz ore
#

Neat! And one final note, in done_task:

 let done_task = case get_by_pos(tasks, num) {
  Ok(task) -> task
  Error(_) -> ""
}

and then you do case with done_task != "". This could be done better with use keyword. It can also help eliminate too deep nesting, but the usage of it can be confusing at the beginning.

Slightly better way would be:

fn done_task(num: Int) -> Result(String, Nil) {
  case load(Tasks), load(Done) {
    Ok(tasks), Ok(done) -> {
      use done_task <- result.try(get_by_pos(tasks, num))

      let res =
        overwrite(
          list.split(tasks, num - 1)
            // this gets rid of the tasks by its pos
            |> fn(x) { #(x.0, list.drop(x.1, 1)) }
            |> fn(x) { list.append(x.0, x.1) },
          list.append(done, [done_task]),
        )

      case res {
        Ok(_) -> Ok(done_task)
        Error(_) -> Error(Nil)
      }
    }
    _, _ -> Error(Nil)
  }
}

You can read about use here: https://tour.gleam.run/everything/#advanced-features-use

#

everything else is looking great to me, good job!

#

for example, with use its possible to do this:

have helper load_all function:

fn load_all(
  next: fn(#(List(String), List(String))) -> Result(a, Nil),
) -> Result(a, Nil) {
  case load(Tasks), load(Done) {
    Ok(tasks), Ok(done) -> next(#(tasks, done))
    _, _ -> Error(Nil)
  }
}

then use it with use keyword in functions like done_task:

fn done_task(num: Int) -> Result(String, Nil) {
  use #(tasks, done) <- load_all()
  use done_task <- result.try(get_by_pos(tasks, num))

  let res =
    overwrite(
      list.split(tasks, num - 1)
        // this gets rid of the tasks by its pos
        |> fn(x) { #(x.0, list.drop(x.1, 1)) }
        |> fn(x) { list.append(x.0, x.1) },
      list.append(done, [done_task]),
    )

  case res {
    Ok(_) -> Ok(done_task)
    Error(_) -> Error(Nil)
  }
}

or reset function:

fn reset(what: Category) -> Result(Nil, Nil) {
  use #(tasks, done) <- load_all()
  case what {
    Tasks -> overwrite([], done)
    Done -> overwrite(tasks, [])
    All -> overwrite([], [])
  }
  |> result.replace_error(Nil)
}
#

or any other function that does case with load(Tasks), load(Done)

dire wyvern
#

use looks incredible, thanks for deeply explaining it to me! I'll do my best to understand it and include it in my next push jigglypuff

topaz ore
dire wyvern
#

so if function in use returns Error, no code after use gets executed?

topaz ore
dire wyvern
#

ooooh

#

I think I get it nowjigglypuff

topaz ore
#

With use, code after it should return Result type

#

wait lemme write more correctly

brisk igloo
#

That's not exactly correct

#

use has nothing to do with the Result type

#

use is just syntax sugar for calling a higher order function

topaz ore
brisk igloo
#

It is often used in combination with functions like result.try from the standard library, but its actual behaviour is entirely dependent on the function you call

brisk igloo
topaz ore
# 
fn done_task(num: Int) -> Result(String, Nil) {
  // `load_all` returns `Result(a, Nil)`. After `use` code must
  // return `Result(a, Nil)`.
  use #(tasks, done) <- load_all()

  // `get_by_pos` returns `Result(String, Nil)`. `get_by_pos` can return
  // `Error(Nil)` which matches previous `use`. After this `use` code must
  // return `Result(String, Nil)`.
  use done_task <- result.try(get_by_pos(tasks, num))

  let res =
    overwrite(
      list.split(tasks, num - 1)
        // this gets rid of the tasks by its pos
        |> fn(x) { #(x.0, list.drop(x.1, 1)) }
        |> fn(x) { list.append(x.0, x.1) },
      list.append(done, [done_task]),
    )

  // returning type matches previous `use` (Result(String, Nil)).
  case res {
    Ok(_) -> Ok(done_task)
    Error(_) -> Error(Nil)
  }
}

is this accurate/correct way of thinking right?

brisk igloo
#

The reason all the code after the use <- load_all code must return Result(a, Nil) is because that's what the signature of the load_all function says. You must pass it a function which returns Result(a, Nil), and in this case, that function is the code below the use

#

Same for the use <- result.try. The type signature of the function you call determines what the code below needs to return

topaz ore
#

thanks for clarifying, I used result.try with use so many times my mind got too used to this XD

dire wyvern
topaz ore